Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: GpsNxMAA/ioOM0RVQqAhzSr/4bnRGCtphhyhoaPuV9eoRL84Qlj3l2bFp1ZfL4aTtiQDWA0Vcz
 78leWMRdTHWoebpd9bUR4EUzrojRUQ0rDQB364883EyrWg8nLgMY3d+MSHDlKAZrvEwJ0dq1qc
 dAPD4GR68vy+ckaupy3TxBgvA/SyExoiQ2dbVGv8/iO6ToRxKSf4EkDP2HaVcKWBsu2OGg8XJ0
 6BfxDP94u959IUQitwnK+n4DUwg5JvJyH+TWpdMbaUNXffSmtcgGragRuS6aZGI1dXpxn23p7l
 rqo=
From:   Damien Le Moal <Damien.LeMoal@wdc.com>
To:     Jan Kara <jack@suse.cz>, "Darrick J. Wong" <djwong@kernel.org>
CC:     "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "hch@infradead.org" <hch@infradead.org>,
        Dave Chinner <david@fromorbit.com>,
        "ceph-devel@vger.kernel.org" <ceph-devel@vger.kernel.org>,
        Chao Yu <yuchao0@huawei.com>,
        "Darrick J. Wong" <darrick.wong@oracle.com>,
        Jaegeuk Kim <jaegeuk@kernel.org>,
        Jeff Layton <jlayton@kernel.org>,
        Johannes Thumshirn <jth@kernel.org>,
        "linux-cifs@vger.kernel.org" <linux-cifs@vger.kernel.org>,
        "linux-ext4@vger.kernel.org" <linux-ext4@vger.kernel.org>,
        "linux-f2fs-devel@lists.sourceforge.net" 
        <linux-f2fs-devel@lists.sourceforge.net>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-xfs@vger.kernel.org" <linux-xfs@vger.kernel.org>,
        Miklos Szeredi <miklos@szeredi.hu>,
        Steve French <sfrench@samba.org>, Ted Tso <tytso@mit.edu>,
        Matthew Wilcox <willy@infradead.org>
Subject: Re: [PATCH 04/13] mm: Add functions to lock invalidate_lock for two
 mappings
Thread-Topic: [PATCH 04/13] mm: Add functions to lock invalidate_lock for two
 mappings
Thread-Index: AQHXUW0PB1DDoKdbbkudtJQ756uiLQ==
Date:   Wed, 26 May 2021 12:11:43 +0000
Message-ID: <DM6PR04MB7081EBE7CE3404AB53F62795E7249@DM6PR04MB7081.namprd04.prod.outlook.com>
References: <20210525125652.20457-1-jack@suse.cz>
 <20210525135100.11221-4-jack@suse.cz> <20210525204805.GM202121@locust>
 <20210526100702.GB30369@quack2.suse.cz>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: fdb28495-13b5-49b7-f647-08d9203f6d4d
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 May 2021 12:11:43.7066
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: OVc1dAwFaGj5Ta5zGOOBGnsk7Aqfocn0DcFT67KOjwNBHSQBKH9d1f9P7d1VSIotitd3WRMTx1w8PXf+6c+ecQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR04MB5995
Precedence: bulk
List-ID: <linux-ext4.vger.kernel.org>
X-Mailing-List: linux-ext4@vger.kernel.org

On 2021/05/26 19:07, Jan Kara wrote:=0A=
> On Tue 25-05-21 13:48:05, Darrick J. Wong wrote:=0A=
>> On Tue, May 25, 2021 at 03:50:41PM +0200, Jan Kara wrote:=0A=
>>> Some operations such as reflinking blocks among files will need to lock=
=0A=
>>> invalidate_lock for two mappings. Add helper functions to do that.=0A=
>>>=0A=
>>> Signed-off-by: Jan Kara <jack@suse.cz>=0A=
>>> ---=0A=
>>>  include/linux/fs.h |  6 ++++++=0A=
>>>  mm/filemap.c       | 38 ++++++++++++++++++++++++++++++++++++++=0A=
>>>  2 files changed, 44 insertions(+)=0A=
>>>=0A=
>>> diff --git a/include/linux/fs.h b/include/linux/fs.h=0A=
>>> index 897238d9f1e0..e6f7447505f5 100644=0A=
>>> --- a/include/linux/fs.h=0A=
>>> +++ b/include/linux/fs.h=0A=
>>> @@ -822,6 +822,12 @@ static inline void inode_lock_shared_nested(struct=
 inode *inode, unsigned subcla=0A=
>>>  void lock_two_nondirectories(struct inode *, struct inode*);=0A=
>>>  void unlock_two_nondirectories(struct inode *, struct inode*);=0A=
>>>  =0A=
>>> +void filemap_invalidate_down_write_two(struct address_space *mapping1,=
=0A=
>>> +				       struct address_space *mapping2);=0A=
>>> +void filemap_invalidate_up_write_two(struct address_space *mapping1,=
=0A=
>>=0A=
>> TBH I find myself wishing that the invalidate_lock used the same=0A=
>> lock/unlock style wrappers that we use for i_rwsem.=0A=
>>=0A=
>> filemap_invalidate_lock(inode1->mapping);=0A=
>> filemap_invalidate_lock_two(inode1->i_mapping, inode2->i_mapping);=0A=
> =0A=
> OK, and filemap_invalidate_lock_shared() for down_read()? I guess that=0A=
> works for me.=0A=
=0A=
What about filemap_invalidate_lock_read() and filemap_invalidate_lock_write=
() ?=0A=
That reminds the down_read()/down_write() without the slightly confusing do=
wn/up.=0A=
=0A=
> =0A=
> 								Honza=0A=
> =0A=
>  =0A=
>> To be fair, I've never been able to keep straight that down means lock=
=0A=
>> and up means unlock.  Ah well, at least you didn't use "p" and "v".=0A=
>>=0A=
>> Mechanically, the changes look ok to me.=0A=
>> Acked-by: Darrick J. Wong <djwong@kernel.org>=0A=
>>=0A=
>> --D=0A=
>>=0A=
>>> +				     struct address_space *mapping2);=0A=
>>> +=0A=
>>> +=0A=
>>>  /*=0A=
>>>   * NOTE: in a 32bit arch with a preemptable kernel and=0A=
>>>   * an UP compile the i_size_read/write must be atomic=0A=
>>> diff --git a/mm/filemap.c b/mm/filemap.c=0A=
>>> index 4d9ec4c6cc34..d3801a9739aa 100644=0A=
>>> --- a/mm/filemap.c=0A=
>>> +++ b/mm/filemap.c=0A=
>>> @@ -1009,6 +1009,44 @@ struct page *__page_cache_alloc(gfp_t gfp)=0A=
>>>  EXPORT_SYMBOL(__page_cache_alloc);=0A=
>>>  #endif=0A=
>>>  =0A=
>>> +/*=0A=
>>> + * filemap_invalidate_down_write_two - lock invalidate_lock for two ma=
ppings=0A=
>>> + *=0A=
>>> + * Lock exclusively invalidate_lock of any passed mapping that is not =
NULL.=0A=
>>> + *=0A=
>>> + * @mapping1: the first mapping to lock=0A=
>>> + * @mapping2: the second mapping to lock=0A=
>>> + */=0A=
>>> +void filemap_invalidate_down_write_two(struct address_space *mapping1,=
=0A=
>>> +				       struct address_space *mapping2)=0A=
>>> +{=0A=
>>> +	if (mapping1 > mapping2)=0A=
>>> +		swap(mapping1, mapping2);=0A=
>>> +	if (mapping1)=0A=
>>> +		down_write(&mapping1->invalidate_lock);=0A=
>>> +	if (mapping2 && mapping1 !=3D mapping2)=0A=
>>> +		down_write_nested(&mapping2->invalidate_lock, 1);=0A=
>>> +}=0A=
>>> +EXPORT_SYMBOL(filemap_invalidate_down_write_two);=0A=
>>> +=0A=
>>> +/*=0A=
>>> + * filemap_invalidate_up_write_two - unlock invalidate_lock for two ma=
ppings=0A=
>>> + *=0A=
>>> + * Unlock exclusive invalidate_lock of any passed mapping that is not =
NULL.=0A=
>>> + *=0A=
>>> + * @mapping1: the first mapping to unlock=0A=
>>> + * @mapping2: the second mapping to unlock=0A=
>>> + */=0A=
>>> +void filemap_invalidate_up_write_two(struct address_space *mapping1,=
=0A=
>>> +				     struct address_space *mapping2)=0A=
>>> +{=0A=
>>> +	if (mapping1)=0A=
>>> +		up_write(&mapping1->invalidate_lock);=0A=
>>> +	if (mapping2 && mapping1 !=3D mapping2)=0A=
>>> +		up_write(&mapping2->invalidate_lock);=0A=
>>> +}=0A=
>>> +EXPORT_SYMBOL(filemap_invalidate_up_write_two);=0A=
>>> +=0A=
>>>  /*=0A=
>>>   * In order to wait for pages to become available there must be=0A=
>>>   * waitqueues associated with pages. By using a hash table of=0A=
>>> -- =0A=
>>> 2.26.2=0A=
>>>=0A=
=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=