| Message ID | 20171129232356.28296-6-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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; > +} > +
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
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.
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!
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 >
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 *);
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(+)