[05/11] fs: add iterate_supers_excl() and iterate_supers_reverse_excl()
diff mbox

Message ID 20171129232356.28296-6-mcgrof@kernel.org
State New
Headers show

Commit Message

Luis Chamberlain Nov. 29, 2017, 11:23 p.m. UTC
There are use cases where we wish to traverse the superblock list
but also capture errors, and in which case we want to avoid having
our callers issue a lock themselves since we can do the locking for
the callers. Provide a iterate_supers_excl() which calls a function
with the write lock held. If an error occurs we capture it and
propagate it.

Likewise there are use cases where we wish to traverse the superblock
list but in reverse order. The new iterate_supers_reverse_excl() helpers
does this but also also captures any errors encountered.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 fs/super.c         | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h |  2 ++
 2 files changed, 93 insertions(+)

Comments

Rafael J. Wysocki Nov. 29, 2017, 11:48 p.m. UTC | #1
On Thu, Nov 30, 2017 at 12:23 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> There are use cases where we wish to traverse the superblock list
> but also capture errors, and in which case we want to avoid having
> our callers issue a lock themselves since we can do the locking for
> the callers. Provide a iterate_supers_excl() which calls a function
> with the write lock held. If an error occurs we capture it and
> propagate it.
>
> Likewise there are use cases where we wish to traverse the superblock
> list but in reverse order. The new iterate_supers_reverse_excl() helpers
> does this but also also captures any errors encountered.
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  fs/super.c         | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h |  2 ++
>  2 files changed, 93 insertions(+)
>
> diff --git a/fs/super.c b/fs/super.c
> index a63513d187e8..885711c1d35b 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -605,6 +605,97 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
>         spin_unlock(&sb_lock);
>  }
>
> +/**
> + *     iterate_supers_excl - exclusively call func for all active superblocks
> + *     @f: function to call
> + *     @arg: argument to pass to it
> + *
> + *     Scans the superblock list and calls given function, passing it
> + *     locked superblock and given argument. Returns 0 unless an error
> + *     occurred on calling the function on any superblock.
> + */
> +int iterate_supers_excl(int (*f)(struct super_block *, void *), void *arg)
> +{
> +       struct super_block *sb, *p = NULL;
> +       int error = 0;
> +
> +       spin_lock(&sb_lock);
> +       list_for_each_entry(sb, &super_blocks, s_list) {
> +               if (hlist_unhashed(&sb->s_instances))
> +                       continue;
> +               sb->s_count++;
> +               spin_unlock(&sb_lock);

Can anything bad happen if the list is modified at this point by a
concurrent thread?

> +
> +               down_write(&sb->s_umount);
> +               if (sb->s_root && (sb->s_flags & SB_BORN)) {
> +                       error = f(sb, arg);
> +                       if (error) {
> +                               up_write(&sb->s_umount);
> +                               spin_lock(&sb_lock);
> +                               __put_super(sb);
> +                               break;
> +                       }
> +               }
> +               up_write(&sb->s_umount);
> +
> +               spin_lock(&sb_lock);
> +               if (p)
> +                       __put_super(p);
> +               p = sb;
> +       }
> +       if (p)
> +               __put_super(p);
> +       spin_unlock(&sb_lock);
> +
> +       return error;
> +}
> +
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luis Chamberlain Nov. 30, 2017, 12:22 a.m. UTC | #2
On Thu, Nov 30, 2017 at 12:48:15AM +0100, Rafael J. Wysocki wrote:
> On Thu, Nov 30, 2017 at 12:23 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > +int iterate_supers_excl(int (*f)(struct super_block *, void *), void *arg)
> > +{
> > +       struct super_block *sb, *p = NULL;
> > +       int error = 0;
> > +
> > +       spin_lock(&sb_lock);
> > +       list_for_each_entry(sb, &super_blocks, s_list) {
> > +               if (hlist_unhashed(&sb->s_instances))
> > +                       continue;
> > +               sb->s_count++;
> > +               spin_unlock(&sb_lock);
> 
> Can anything bad happen if the list is modified at this point by a
> concurrent thread?

The race is theoretical and applies to all users of iterate_supers() as well.

Its certainly worth considering, however given other code is implicated its
not a *new* issue or race. Its the best we can do with the current design.

That said, as I looked at all this code I considered that perhaps super_blocks
deserves its own RCU lock to enable us to do full swap operations on the list,
without having to do these nasty releases in between.

If that's possible / desirable I'd consider it a welcomed future optimization,
and I could give it a shot, however its unclear if this is a requirement for
this feature at this time.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dave Chinner Nov. 30, 2017, 1:34 a.m. UTC | #3
On Thu, Nov 30, 2017 at 12:48:15AM +0100, Rafael J. Wysocki wrote:
> On Thu, Nov 30, 2017 at 12:23 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > There are use cases where we wish to traverse the superblock list
> > but also capture errors, and in which case we want to avoid having
> > our callers issue a lock themselves since we can do the locking for
> > the callers. Provide a iterate_supers_excl() which calls a function
> > with the write lock held. If an error occurs we capture it and
> > propagate it.
> >
> > Likewise there are use cases where we wish to traverse the superblock
> > list but in reverse order. The new iterate_supers_reverse_excl() helpers
> > does this but also also captures any errors encountered.
> >
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > ---
> >  fs/super.c         | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/fs.h |  2 ++
> >  2 files changed, 93 insertions(+)
> >
> > diff --git a/fs/super.c b/fs/super.c
> > index a63513d187e8..885711c1d35b 100644
> > --- a/fs/super.c
> > +++ b/fs/super.c
> > @@ -605,6 +605,97 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
> >         spin_unlock(&sb_lock);
> >  }
> >
> > +/**
> > + *     iterate_supers_excl - exclusively call func for all active superblocks
> > + *     @f: function to call
> > + *     @arg: argument to pass to it
> > + *
> > + *     Scans the superblock list and calls given function, passing it
> > + *     locked superblock and given argument. Returns 0 unless an error
> > + *     occurred on calling the function on any superblock.
> > + */
> > +int iterate_supers_excl(int (*f)(struct super_block *, void *), void *arg)
> > +{
> > +       struct super_block *sb, *p = NULL;
> > +       int error = 0;
> > +
> > +       spin_lock(&sb_lock);
> > +       list_for_each_entry(sb, &super_blocks, s_list) {
> > +               if (hlist_unhashed(&sb->s_instances))
> > +                       continue;
> > +               sb->s_count++;
> > +               spin_unlock(&sb_lock);
> 
> Can anything bad happen if the list is modified at this point by a
> concurrent thread?

No. We have a valid reference to sb->s_count and that keeps it on
the list while we have the lock dropped. The sb reference isn't
dropped until we've iterated to the next sb on the list and taken a
reference to that, hence it's safe to drop and regain the list lock
without needing to restart the iteration.

> > +
> > +               down_write(&sb->s_umount);
> > +               if (sb->s_root && (sb->s_flags & SB_BORN)) {
> > +                       error = f(sb, arg);
> > +                       if (error) {
> > +                               up_write(&sb->s_umount);
> > +                               spin_lock(&sb_lock);
> > +                               __put_super(sb);
> > +                               break;
> > +                       }
> > +               }
> > +               up_write(&sb->s_umount);
> > +
> > +               spin_lock(&sb_lock);
> > +               if (p)
> > +                       __put_super(p);
> > +               p = sb;

This code here is what drops the reference to the previous sb
we've iterated past.

FWIW, this "hold until next is held" iteration pattern is used
frequently for inodes, dentries, and other reference counted VFS
objects so we can iterate the list without needing to hold the
list lock for the entire iteration....

Cheers,

Dave.
Rafael J. Wysocki Nov. 30, 2017, 1:40 a.m. UTC | #4
On Thu, Nov 30, 2017 at 2:34 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Thu, Nov 30, 2017 at 12:48:15AM +0100, Rafael J. Wysocki wrote:
>> On Thu, Nov 30, 2017 at 12:23 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> > There are use cases where we wish to traverse the superblock list
>> > but also capture errors, and in which case we want to avoid having
>> > our callers issue a lock themselves since we can do the locking for
>> > the callers. Provide a iterate_supers_excl() which calls a function
>> > with the write lock held. If an error occurs we capture it and
>> > propagate it.
>> >
>> > Likewise there are use cases where we wish to traverse the superblock
>> > list but in reverse order. The new iterate_supers_reverse_excl() helpers
>> > does this but also also captures any errors encountered.
>> >
>> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
>> > ---
>> >  fs/super.c         | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  include/linux/fs.h |  2 ++
>> >  2 files changed, 93 insertions(+)
>> >
>> > diff --git a/fs/super.c b/fs/super.c
>> > index a63513d187e8..885711c1d35b 100644
>> > --- a/fs/super.c
>> > +++ b/fs/super.c
>> > @@ -605,6 +605,97 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
>> >         spin_unlock(&sb_lock);
>> >  }
>> >
>> > +/**
>> > + *     iterate_supers_excl - exclusively call func for all active superblocks
>> > + *     @f: function to call
>> > + *     @arg: argument to pass to it
>> > + *
>> > + *     Scans the superblock list and calls given function, passing it
>> > + *     locked superblock and given argument. Returns 0 unless an error
>> > + *     occurred on calling the function on any superblock.
>> > + */
>> > +int iterate_supers_excl(int (*f)(struct super_block *, void *), void *arg)
>> > +{
>> > +       struct super_block *sb, *p = NULL;
>> > +       int error = 0;
>> > +
>> > +       spin_lock(&sb_lock);
>> > +       list_for_each_entry(sb, &super_blocks, s_list) {
>> > +               if (hlist_unhashed(&sb->s_instances))
>> > +                       continue;
>> > +               sb->s_count++;
>> > +               spin_unlock(&sb_lock);
>>
>> Can anything bad happen if the list is modified at this point by a
>> concurrent thread?
>
> No. We have a valid reference to sb->s_count and that keeps it on
> the list while we have the lock dropped. The sb reference isn't
> dropped until we've iterated to the next sb on the list and taken a
> reference to that, hence it's safe to drop and regain the list lock
> without needing to restart the iteration.
>
>> > +
>> > +               down_write(&sb->s_umount);
>> > +               if (sb->s_root && (sb->s_flags & SB_BORN)) {
>> > +                       error = f(sb, arg);
>> > +                       if (error) {
>> > +                               up_write(&sb->s_umount);
>> > +                               spin_lock(&sb_lock);
>> > +                               __put_super(sb);
>> > +                               break;
>> > +                       }
>> > +               }
>> > +               up_write(&sb->s_umount);
>> > +
>> > +               spin_lock(&sb_lock);
>> > +               if (p)
>> > +                       __put_super(p);
>> > +               p = sb;
>
> This code here is what drops the reference to the previous sb
> we've iterated past.
>
> FWIW, this "hold until next is held" iteration pattern is used
> frequently for inodes, dentries, and other reference counted VFS
> objects so we can iterate the list without needing to hold the
> list lock for the entire iteration....

OK, thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kara Nov. 30, 2017, 4:57 p.m. UTC | #5
On Wed 29-11-17 15:23:50, Luis R. Rodriguez wrote:
> There are use cases where we wish to traverse the superblock list
> but also capture errors, and in which case we want to avoid having
> our callers issue a lock themselves since we can do the locking for
> the callers. Provide a iterate_supers_excl() which calls a function
> with the write lock held. If an error occurs we capture it and
> propagate it.
> 
> Likewise there are use cases where we wish to traverse the superblock
> list but in reverse order. The new iterate_supers_reverse_excl() helpers
> does this but also also captures any errors encountered.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza


> ---
>  fs/super.c         | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h |  2 ++
>  2 files changed, 93 insertions(+)
> 
> diff --git a/fs/super.c b/fs/super.c
> index a63513d187e8..885711c1d35b 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -605,6 +605,97 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
>  	spin_unlock(&sb_lock);
>  }
>  
> +/**
> + *	iterate_supers_excl - exclusively call func for all active superblocks
> + *	@f: function to call
> + *	@arg: argument to pass to it
> + *
> + *	Scans the superblock list and calls given function, passing it
> + *	locked superblock and given argument. Returns 0 unless an error
> + *	occurred on calling the function on any superblock.
> + */
> +int iterate_supers_excl(int (*f)(struct super_block *, void *), void *arg)
> +{
> +	struct super_block *sb, *p = NULL;
> +	int error = 0;
> +
> +	spin_lock(&sb_lock);
> +	list_for_each_entry(sb, &super_blocks, s_list) {
> +		if (hlist_unhashed(&sb->s_instances))
> +			continue;
> +		sb->s_count++;
> +		spin_unlock(&sb_lock);
> +
> +		down_write(&sb->s_umount);
> +		if (sb->s_root && (sb->s_flags & SB_BORN)) {
> +			error = f(sb, arg);
> +			if (error) {
> +				up_write(&sb->s_umount);
> +				spin_lock(&sb_lock);
> +				__put_super(sb);
> +				break;
> +			}
> +		}
> +		up_write(&sb->s_umount);
> +
> +		spin_lock(&sb_lock);
> +		if (p)
> +			__put_super(p);
> +		p = sb;
> +	}
> +	if (p)
> +		__put_super(p);
> +	spin_unlock(&sb_lock);
> +
> +	return error;
> +}
> +
> +/**
> + *	iterate_supers_reverse_excl - exclusively calls func in reverse order
> + *	@f: function to call
> + *	@arg: argument to pass to it
> + *
> + *	Scans the superblock list and calls given function, passing it
> + *	locked superblock and given argument, in reverse order, and holding
> + *	the s_umount write lock. Returns if an error occurred.
> + */
> +int iterate_supers_reverse_excl(int (*f)(struct super_block *, void *),
> +					 void *arg)
> +{
> +	struct super_block *sb, *p = NULL;
> +	int error = 0;
> +
> +	spin_lock(&sb_lock);
> +	list_for_each_entry_reverse(sb, &super_blocks, s_list) {
> +		if (hlist_unhashed(&sb->s_instances))
> +			continue;
> +		sb->s_count++;
> +		spin_unlock(&sb_lock);
> +
> +		down_write(&sb->s_umount);
> +		if (sb->s_root && (sb->s_flags & SB_BORN)) {
> +			error = f(sb, arg);
> +			if (error) {
> +				up_write(&sb->s_umount);
> +				spin_lock(&sb_lock);
> +				__put_super(sb);
> +				break;
> +			}
> +		}
> +		up_write(&sb->s_umount);
> +
> +		spin_lock(&sb_lock);
> +		if (p)
> +			__put_super(p);
> +		p = sb;
> +	}
> +	if (p)
> +		__put_super(p);
> +	spin_unlock(&sb_lock);
> +
> +	return error;
> +}
> +
>  /**
>   *	iterate_supers_type - call function for superblocks of given type
>   *	@type: fs type
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 107725b20fad..fe90b6542697 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3164,6 +3164,8 @@ extern struct super_block *get_active_super(struct block_device *bdev);
>  extern void drop_super(struct super_block *sb);
>  extern void drop_super_exclusive(struct super_block *sb);
>  extern void iterate_supers(void (*)(struct super_block *, void *), void *);
> +extern int iterate_supers_excl(int (*f)(struct super_block *, void *), void *arg);
> +extern int iterate_supers_reverse_excl(int (*)(struct super_block *, void *), void *);
>  extern void iterate_supers_type(struct file_system_type *,
>  			        void (*)(struct super_block *, void *), void *);
>  
> -- 
> 2.15.0
>

Patch
diff mbox

diff --git a/fs/super.c b/fs/super.c
index a63513d187e8..885711c1d35b 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -605,6 +605,97 @@  void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
 	spin_unlock(&sb_lock);
 }
 
+/**
+ *	iterate_supers_excl - exclusively call func for all active superblocks
+ *	@f: function to call
+ *	@arg: argument to pass to it
+ *
+ *	Scans the superblock list and calls given function, passing it
+ *	locked superblock and given argument. Returns 0 unless an error
+ *	occurred on calling the function on any superblock.
+ */
+int iterate_supers_excl(int (*f)(struct super_block *, void *), void *arg)
+{
+	struct super_block *sb, *p = NULL;
+	int error = 0;
+
+	spin_lock(&sb_lock);
+	list_for_each_entry(sb, &super_blocks, s_list) {
+		if (hlist_unhashed(&sb->s_instances))
+			continue;
+		sb->s_count++;
+		spin_unlock(&sb_lock);
+
+		down_write(&sb->s_umount);
+		if (sb->s_root && (sb->s_flags & SB_BORN)) {
+			error = f(sb, arg);
+			if (error) {
+				up_write(&sb->s_umount);
+				spin_lock(&sb_lock);
+				__put_super(sb);
+				break;
+			}
+		}
+		up_write(&sb->s_umount);
+
+		spin_lock(&sb_lock);
+		if (p)
+			__put_super(p);
+		p = sb;
+	}
+	if (p)
+		__put_super(p);
+	spin_unlock(&sb_lock);
+
+	return error;
+}
+
+/**
+ *	iterate_supers_reverse_excl - exclusively calls func in reverse order
+ *	@f: function to call
+ *	@arg: argument to pass to it
+ *
+ *	Scans the superblock list and calls given function, passing it
+ *	locked superblock and given argument, in reverse order, and holding
+ *	the s_umount write lock. Returns if an error occurred.
+ */
+int iterate_supers_reverse_excl(int (*f)(struct super_block *, void *),
+					 void *arg)
+{
+	struct super_block *sb, *p = NULL;
+	int error = 0;
+
+	spin_lock(&sb_lock);
+	list_for_each_entry_reverse(sb, &super_blocks, s_list) {
+		if (hlist_unhashed(&sb->s_instances))
+			continue;
+		sb->s_count++;
+		spin_unlock(&sb_lock);
+
+		down_write(&sb->s_umount);
+		if (sb->s_root && (sb->s_flags & SB_BORN)) {
+			error = f(sb, arg);
+			if (error) {
+				up_write(&sb->s_umount);
+				spin_lock(&sb_lock);
+				__put_super(sb);
+				break;
+			}
+		}
+		up_write(&sb->s_umount);
+
+		spin_lock(&sb_lock);
+		if (p)
+			__put_super(p);
+		p = sb;
+	}
+	if (p)
+		__put_super(p);
+	spin_unlock(&sb_lock);
+
+	return error;
+}
+
 /**
  *	iterate_supers_type - call function for superblocks of given type
  *	@type: fs type
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 107725b20fad..fe90b6542697 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3164,6 +3164,8 @@  extern struct super_block *get_active_super(struct block_device *bdev);
 extern void drop_super(struct super_block *sb);
 extern void drop_super_exclusive(struct super_block *sb);
 extern void iterate_supers(void (*)(struct super_block *, void *), void *);
+extern int iterate_supers_excl(int (*f)(struct super_block *, void *), void *arg);
+extern int iterate_supers_reverse_excl(int (*)(struct super_block *, void *), void *);
 extern void iterate_supers_type(struct file_system_type *,
 			        void (*)(struct super_block *, void *), void *);