diff mbox series

[04/13] mm: Add functions to lock invalidate_lock for two mappings

Message ID 20210525135100.11221-4-jack@suse.cz (mailing list archive)
State New
Headers show
Series fs: Hole punch vs page cache filling races | expand

Commit Message

Jan Kara May 25, 2021, 1:50 p.m. UTC
Some operations such as reflinking blocks among files will need to lock
invalidate_lock for two mappings. Add helper functions to do that.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 include/linux/fs.h |  6 ++++++
 mm/filemap.c       | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

Comments

Darrick J. Wong May 25, 2021, 8:48 p.m. UTC | #1
On Tue, May 25, 2021 at 03:50:41PM +0200, Jan Kara wrote:
> Some operations such as reflinking blocks among files will need to lock
> invalidate_lock for two mappings. Add helper functions to do that.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  include/linux/fs.h |  6 ++++++
>  mm/filemap.c       | 38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 897238d9f1e0..e6f7447505f5 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -822,6 +822,12 @@ static inline void inode_lock_shared_nested(struct inode *inode, unsigned subcla
>  void lock_two_nondirectories(struct inode *, struct inode*);
>  void unlock_two_nondirectories(struct inode *, struct inode*);
>  
> +void filemap_invalidate_down_write_two(struct address_space *mapping1,
> +				       struct address_space *mapping2);
> +void filemap_invalidate_up_write_two(struct address_space *mapping1,

TBH I find myself wishing that the invalidate_lock used the same
lock/unlock style wrappers that we use for i_rwsem.

filemap_invalidate_lock(inode1->mapping);
filemap_invalidate_lock_two(inode1->i_mapping, inode2->i_mapping);

To be fair, I've never been able to keep straight that down means lock
and up means unlock.  Ah well, at least you didn't use "p" and "v".

Mechanically, the changes look ok to me.
Acked-by: Darrick J. Wong <djwong@kernel.org>

--D

> +				     struct address_space *mapping2);
> +
> +
>  /*
>   * NOTE: in a 32bit arch with a preemptable kernel and
>   * an UP compile the i_size_read/write must be atomic
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 4d9ec4c6cc34..d3801a9739aa 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1009,6 +1009,44 @@ struct page *__page_cache_alloc(gfp_t gfp)
>  EXPORT_SYMBOL(__page_cache_alloc);
>  #endif
>  
> +/*
> + * filemap_invalidate_down_write_two - lock invalidate_lock for two mappings
> + *
> + * Lock exclusively invalidate_lock of any passed mapping that is not NULL.
> + *
> + * @mapping1: the first mapping to lock
> + * @mapping2: the second mapping to lock
> + */
> +void filemap_invalidate_down_write_two(struct address_space *mapping1,
> +				       struct address_space *mapping2)
> +{
> +	if (mapping1 > mapping2)
> +		swap(mapping1, mapping2);
> +	if (mapping1)
> +		down_write(&mapping1->invalidate_lock);
> +	if (mapping2 && mapping1 != mapping2)
> +		down_write_nested(&mapping2->invalidate_lock, 1);
> +}
> +EXPORT_SYMBOL(filemap_invalidate_down_write_two);
> +
> +/*
> + * filemap_invalidate_up_write_two - unlock invalidate_lock for two mappings
> + *
> + * Unlock exclusive invalidate_lock of any passed mapping that is not NULL.
> + *
> + * @mapping1: the first mapping to unlock
> + * @mapping2: the second mapping to unlock
> + */
> +void filemap_invalidate_up_write_two(struct address_space *mapping1,
> +				     struct address_space *mapping2)
> +{
> +	if (mapping1)
> +		up_write(&mapping1->invalidate_lock);
> +	if (mapping2 && mapping1 != mapping2)
> +		up_write(&mapping2->invalidate_lock);
> +}
> +EXPORT_SYMBOL(filemap_invalidate_up_write_two);
> +
>  /*
>   * In order to wait for pages to become available there must be
>   * waitqueues associated with pages. By using a hash table of
> -- 
> 2.26.2
>
Jan Kara May 26, 2021, 10:07 a.m. UTC | #2
On Tue 25-05-21 13:48:05, Darrick J. Wong wrote:
> On Tue, May 25, 2021 at 03:50:41PM +0200, Jan Kara wrote:
> > Some operations such as reflinking blocks among files will need to lock
> > invalidate_lock for two mappings. Add helper functions to do that.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  include/linux/fs.h |  6 ++++++
> >  mm/filemap.c       | 38 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 44 insertions(+)
> > 
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 897238d9f1e0..e6f7447505f5 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -822,6 +822,12 @@ static inline void inode_lock_shared_nested(struct inode *inode, unsigned subcla
> >  void lock_two_nondirectories(struct inode *, struct inode*);
> >  void unlock_two_nondirectories(struct inode *, struct inode*);
> >  
> > +void filemap_invalidate_down_write_two(struct address_space *mapping1,
> > +				       struct address_space *mapping2);
> > +void filemap_invalidate_up_write_two(struct address_space *mapping1,
> 
> TBH I find myself wishing that the invalidate_lock used the same
> lock/unlock style wrappers that we use for i_rwsem.
> 
> filemap_invalidate_lock(inode1->mapping);
> filemap_invalidate_lock_two(inode1->i_mapping, inode2->i_mapping);

OK, and filemap_invalidate_lock_shared() for down_read()? I guess that
works for me.

								Honza

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

What about filemap_invalidate_lock_read() and filemap_invalidate_lock_write() ?
That reminds the down_read()/down_write() without the slightly confusing down/up.

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

Well, if we go for lock wrappers as Darrick suggested, I'd mirror naming
used for inode_lock(). That is IMO the least confusing option... And that
naming has _lock and _lock_shared suffixes.

								Honza
Darrick J. Wong May 26, 2021, 3:25 p.m. UTC | #5
On Wed, May 26, 2021 at 03:45:18PM +0200, Jan Kara wrote:
> On Wed 26-05-21 12:11:43, Damien Le Moal wrote:
> > On 2021/05/26 19:07, Jan Kara wrote:
> > > On Tue 25-05-21 13:48:05, Darrick J. Wong wrote:
> > >> On Tue, May 25, 2021 at 03:50:41PM +0200, Jan Kara wrote:
> > >>> Some operations such as reflinking blocks among files will need to lock
> > >>> invalidate_lock for two mappings. Add helper functions to do that.
> > >>>
> > >>> Signed-off-by: Jan Kara <jack@suse.cz>
> > >>> ---
> > >>>  include/linux/fs.h |  6 ++++++
> > >>>  mm/filemap.c       | 38 ++++++++++++++++++++++++++++++++++++++
> > >>>  2 files changed, 44 insertions(+)
> > >>>
> > >>> diff --git a/include/linux/fs.h b/include/linux/fs.h
> > >>> index 897238d9f1e0..e6f7447505f5 100644
> > >>> --- a/include/linux/fs.h
> > >>> +++ b/include/linux/fs.h
> > >>> @@ -822,6 +822,12 @@ static inline void inode_lock_shared_nested(struct inode *inode, unsigned subcla
> > >>>  void lock_two_nondirectories(struct inode *, struct inode*);
> > >>>  void unlock_two_nondirectories(struct inode *, struct inode*);
> > >>>  
> > >>> +void filemap_invalidate_down_write_two(struct address_space *mapping1,
> > >>> +				       struct address_space *mapping2);
> > >>> +void filemap_invalidate_up_write_two(struct address_space *mapping1,
> > >>
> > >> TBH I find myself wishing that the invalidate_lock used the same
> > >> lock/unlock style wrappers that we use for i_rwsem.
> > >>
> > >> filemap_invalidate_lock(inode1->mapping);
> > >> filemap_invalidate_lock_two(inode1->i_mapping, inode2->i_mapping);
> > > 
> > > OK, and filemap_invalidate_lock_shared() for down_read()? I guess that
> > > works for me.
> > 
> > What about filemap_invalidate_lock_read() and filemap_invalidate_lock_write() ?
> > That reminds the down_read()/down_write() without the slightly confusing down/up.
> 
> Well, if we go for lock wrappers as Darrick suggested, I'd mirror naming
> used for inode_lock(). That is IMO the least confusing option... And that
> naming has _lock and _lock_shared suffixes.

I'd like filemap_invalidate_lock and filemap_invalidate_lock_shared.

--D

> 
> 								Honza
> 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
diff mbox series

Patch

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 897238d9f1e0..e6f7447505f5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -822,6 +822,12 @@  static inline void inode_lock_shared_nested(struct inode *inode, unsigned subcla
 void lock_two_nondirectories(struct inode *, struct inode*);
 void unlock_two_nondirectories(struct inode *, struct inode*);
 
+void filemap_invalidate_down_write_two(struct address_space *mapping1,
+				       struct address_space *mapping2);
+void filemap_invalidate_up_write_two(struct address_space *mapping1,
+				     struct address_space *mapping2);
+
+
 /*
  * NOTE: in a 32bit arch with a preemptable kernel and
  * an UP compile the i_size_read/write must be atomic
diff --git a/mm/filemap.c b/mm/filemap.c
index 4d9ec4c6cc34..d3801a9739aa 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1009,6 +1009,44 @@  struct page *__page_cache_alloc(gfp_t gfp)
 EXPORT_SYMBOL(__page_cache_alloc);
 #endif
 
+/*
+ * filemap_invalidate_down_write_two - lock invalidate_lock for two mappings
+ *
+ * Lock exclusively invalidate_lock of any passed mapping that is not NULL.
+ *
+ * @mapping1: the first mapping to lock
+ * @mapping2: the second mapping to lock
+ */
+void filemap_invalidate_down_write_two(struct address_space *mapping1,
+				       struct address_space *mapping2)
+{
+	if (mapping1 > mapping2)
+		swap(mapping1, mapping2);
+	if (mapping1)
+		down_write(&mapping1->invalidate_lock);
+	if (mapping2 && mapping1 != mapping2)
+		down_write_nested(&mapping2->invalidate_lock, 1);
+}
+EXPORT_SYMBOL(filemap_invalidate_down_write_two);
+
+/*
+ * filemap_invalidate_up_write_two - unlock invalidate_lock for two mappings
+ *
+ * Unlock exclusive invalidate_lock of any passed mapping that is not NULL.
+ *
+ * @mapping1: the first mapping to unlock
+ * @mapping2: the second mapping to unlock
+ */
+void filemap_invalidate_up_write_two(struct address_space *mapping1,
+				     struct address_space *mapping2)
+{
+	if (mapping1)
+		up_write(&mapping1->invalidate_lock);
+	if (mapping2 && mapping1 != mapping2)
+		up_write(&mapping2->invalidate_lock);
+}
+EXPORT_SYMBOL(filemap_invalidate_up_write_two);
+
 /*
  * In order to wait for pages to become available there must be
  * waitqueues associated with pages. By using a hash table of