| Message ID | 20230619111832.3886-1-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] fs: Provide helpers for manipulating sb->s_readonly_remount | expand |
On Mon, 19 Jun 2023 13:18:32 +0200, Jan Kara wrote: > Provide helpers to set and clear sb->s_readonly_remount including > appropriate memory barriers. Also use this opportunity to document what > the barriers pair with and why they are needed. > > I'm traveling back from Vienna to Berlin today so will back online completely tomorrow. This looks good to me now. Thanks for the nice cleanup. Fwiw, it could've also waited until after the merge window but now is obviously fine too. --- Applied to the vfs.misc branch of the vfs/vfs.git tree. Patches in the vfs.misc branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.misc [1/1] fs: Provide helpers for manipulating sb->s_readonly_remount https://git.kernel.org/vfs/vfs/c/5cf8f23baf5f
On Mon, Jun 19, 2023 at 01:18:32PM +0200, Jan Kara wrote: > Provide helpers to set and clear sb->s_readonly_remount including > appropriate memory barriers. Also use this opportunity to document what > the barriers pair with and why they are needed. > > Suggested-by: Dave Chinner <david@fromorbit.com> > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/internal.h | 34 ++++++++++++++++++++++++++++++++++ > fs/namespace.c | 10 ++++------ > fs/super.c | 17 ++++++----------- > include/linux/fs.h | 2 +- > 4 files changed, 45 insertions(+), 18 deletions(-) > > diff --git a/fs/internal.h b/fs/internal.h > index bd3b2810a36b..e206eb58bd3e 100644 > --- a/fs/internal.h > +++ b/fs/internal.h > @@ -120,6 +120,40 @@ void put_super(struct super_block *sb); > extern bool mount_capable(struct fs_context *); > int sb_init_dio_done_wq(struct super_block *sb); > > +/* > + * Prepare superblock for changing its read-only state (i.e., either remount > + * read-write superblock read-only or vice versa). After this function returns > + * mnt_is_readonly() will return true for any mount of the superblock if its > + * caller is able to observe any changes done by the remount. This holds until > + * sb_end_ro_state_change() is called. > + */ > +static inline void sb_start_ro_state_change(struct super_block *sb) > +{ > + WRITE_ONCE(sb->s_readonly_remount, 1); > + /* > + * For RO->RW transition, the barrier pairs with the barrier in > + * mnt_is_readonly() making sure if mnt_is_readonly() sees SB_RDONLY > + * cleared, it will see s_readonly_remount set. > + * For RW->RO transition, the barrier pairs with the barrier in > + * __mnt_want_write() before the mnt_is_readonly() check. The barrier > + * makes sure if __mnt_want_write() sees MNT_WRITE_HOLD already > + * cleared, it will see s_readonly_remount set. > + */ > + smp_wmb(); > +} Can you please also update mnt_is_readonly/__mnt_want_write to indicate that there is a pairing with this helper from those functions? > + > +/* > + * Ends section changing read-only state of the superblock. After this function > + * returns if mnt_is_readonly() returns false, the caller will be able to > + * observe all the changes remount did to the superblock. > + */ > +static inline void sb_end_ro_state_change(struct super_block *sb) > +{ > + /* The barrier pairs with the barrier in mnt_is_readonly() */ > + smp_wmb(); > + WRITE_ONCE(sb->s_readonly_remount, 0); > +} /* * This barrier provides release semantics that pair with * the smp_rmb() acquire semantics in mnt_is_readonly(). * This barrier pair ensure that when mnt_is_readonly() sees * 0 for sb->s_readonly_remount, it will also see all the * preceding flag changes that were made during the RO state * change. */ And a comment in mnt_is_readonly() to indicate that it also pairs with sb_end_ro_state_change() in a different way to the barriers in sb_start_ro_state_change(), __mnt_want_write(), MNT_WRITE_HOLD, etc. Memory barriers need clear, concise documentation, otherwise they are impossible to understand from just reading the code... Cheers, Dave.
On Tue 20-06-23 09:25:05, Dave Chinner wrote: > On Mon, Jun 19, 2023 at 01:18:32PM +0200, Jan Kara wrote: > > Provide helpers to set and clear sb->s_readonly_remount including > > appropriate memory barriers. Also use this opportunity to document what > > the barriers pair with and why they are needed. > > > > Suggested-by: Dave Chinner <david@fromorbit.com> > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/internal.h | 34 ++++++++++++++++++++++++++++++++++ > > fs/namespace.c | 10 ++++------ > > fs/super.c | 17 ++++++----------- > > include/linux/fs.h | 2 +- > > 4 files changed, 45 insertions(+), 18 deletions(-) > > > > diff --git a/fs/internal.h b/fs/internal.h > > index bd3b2810a36b..e206eb58bd3e 100644 > > --- a/fs/internal.h > > +++ b/fs/internal.h > > @@ -120,6 +120,40 @@ void put_super(struct super_block *sb); > > extern bool mount_capable(struct fs_context *); > > int sb_init_dio_done_wq(struct super_block *sb); > > > > +/* > > + * Prepare superblock for changing its read-only state (i.e., either remount > > + * read-write superblock read-only or vice versa). After this function returns > > + * mnt_is_readonly() will return true for any mount of the superblock if its > > + * caller is able to observe any changes done by the remount. This holds until > > + * sb_end_ro_state_change() is called. > > + */ > > +static inline void sb_start_ro_state_change(struct super_block *sb) > > +{ > > + WRITE_ONCE(sb->s_readonly_remount, 1); > > + /* > > + * For RO->RW transition, the barrier pairs with the barrier in > > + * mnt_is_readonly() making sure if mnt_is_readonly() sees SB_RDONLY > > + * cleared, it will see s_readonly_remount set. > > + * For RW->RO transition, the barrier pairs with the barrier in > > + * __mnt_want_write() before the mnt_is_readonly() check. The barrier > > + * makes sure if __mnt_want_write() sees MNT_WRITE_HOLD already > > + * cleared, it will see s_readonly_remount set. > > + */ > > + smp_wmb(); > > +} > > Can you please also update mnt_is_readonly/__mnt_want_write to > indicate that there is a pairing with this helper from those > functions? Ok, I've expanded / updated the comments there. > > + > > +/* > > + * Ends section changing read-only state of the superblock. After this function > > + * returns if mnt_is_readonly() returns false, the caller will be able to > > + * observe all the changes remount did to the superblock. > > + */ > > +static inline void sb_end_ro_state_change(struct super_block *sb) > > +{ > > + /* The barrier pairs with the barrier in mnt_is_readonly() */ > > + smp_wmb(); > > + WRITE_ONCE(sb->s_readonly_remount, 0); > > +} > > /* > * This barrier provides release semantics that pair with > * the smp_rmb() acquire semantics in mnt_is_readonly(). > * This barrier pair ensure that when mnt_is_readonly() sees > * 0 for sb->s_readonly_remount, it will also see all the > * preceding flag changes that were made during the RO state > * change. > */ > > And a comment in mnt_is_readonly() to indicate that it also pairs > with sb_end_ro_state_change() in a different way to the barriers in > sb_start_ro_state_change(), __mnt_want_write(), MNT_WRITE_HOLD, etc. Thanks! I've added your comment and expanded more on pairing with sb_end_ro_state_change(). Honza
diff --git a/fs/internal.h b/fs/internal.h index bd3b2810a36b..e206eb58bd3e 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -120,6 +120,40 @@ void put_super(struct super_block *sb); extern bool mount_capable(struct fs_context *); int sb_init_dio_done_wq(struct super_block *sb); +/* + * Prepare superblock for changing its read-only state (i.e., either remount + * read-write superblock read-only or vice versa). After this function returns + * mnt_is_readonly() will return true for any mount of the superblock if its + * caller is able to observe any changes done by the remount. This holds until + * sb_end_ro_state_change() is called. + */ +static inline void sb_start_ro_state_change(struct super_block *sb) +{ + WRITE_ONCE(sb->s_readonly_remount, 1); + /* + * For RO->RW transition, the barrier pairs with the barrier in + * mnt_is_readonly() making sure if mnt_is_readonly() sees SB_RDONLY + * cleared, it will see s_readonly_remount set. + * For RW->RO transition, the barrier pairs with the barrier in + * __mnt_want_write() before the mnt_is_readonly() check. The barrier + * makes sure if __mnt_want_write() sees MNT_WRITE_HOLD already + * cleared, it will see s_readonly_remount set. + */ + smp_wmb(); +} + +/* + * Ends section changing read-only state of the superblock. After this function + * returns if mnt_is_readonly() returns false, the caller will be able to + * observe all the changes remount did to the superblock. + */ +static inline void sb_end_ro_state_change(struct super_block *sb) +{ + /* The barrier pairs with the barrier in mnt_is_readonly() */ + smp_wmb(); + WRITE_ONCE(sb->s_readonly_remount, 0); +} + /* * open.c */ diff --git a/fs/namespace.c b/fs/namespace.c index 54847db5b819..2eff091df549 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -309,9 +309,9 @@ static unsigned int mnt_get_writers(struct mount *mnt) static int mnt_is_readonly(struct vfsmount *mnt) { - if (mnt->mnt_sb->s_readonly_remount) + if (READ_ONCE(mnt->mnt_sb->s_readonly_remount)) return 1; - /* Order wrt setting s_flags/s_readonly_remount in do_remount() */ + /* Order wrt barriers in sb_{start,end}_ro_state_change() */ smp_rmb(); return __mnt_is_readonly(mnt); } @@ -588,10 +588,8 @@ int sb_prepare_remount_readonly(struct super_block *sb) if (!err && atomic_long_read(&sb->s_remove_count)) err = -EBUSY; - if (!err) { - sb->s_readonly_remount = 1; - smp_wmb(); - } + if (!err) + sb_start_ro_state_change(sb); list_for_each_entry(mnt, &sb->s_mounts, mnt_instance) { if (mnt->mnt.mnt_flags & MNT_WRITE_HOLD) mnt->mnt.mnt_flags &= ~MNT_WRITE_HOLD; diff --git a/fs/super.c b/fs/super.c index 6cd64961aa07..8a39902b859f 100644 --- a/fs/super.c +++ b/fs/super.c @@ -944,8 +944,7 @@ int reconfigure_super(struct fs_context *fc) */ if (remount_ro) { if (force) { - sb->s_readonly_remount = 1; - smp_wmb(); + sb_start_ro_state_change(sb); } else { retval = sb_prepare_remount_readonly(sb); if (retval) @@ -953,12 +952,10 @@ int reconfigure_super(struct fs_context *fc) } } else if (remount_rw) { /* - * We set s_readonly_remount here to protect filesystem's - * reconfigure code from writes from userspace until - * reconfigure finishes. + * Protect filesystem's reconfigure code from writes from + * userspace until reconfigure finishes. */ - sb->s_readonly_remount = 1; - smp_wmb(); + sb_start_ro_state_change(sb); } if (fc->ops->reconfigure) { @@ -974,9 +971,7 @@ int reconfigure_super(struct fs_context *fc) WRITE_ONCE(sb->s_flags, ((sb->s_flags & ~fc->sb_flags_mask) | (fc->sb_flags & fc->sb_flags_mask))); - /* Needs to be ordered wrt mnt_is_readonly() */ - smp_wmb(); - sb->s_readonly_remount = 0; + sb_end_ro_state_change(sb); /* * Some filesystems modify their metadata via some other path than the @@ -991,7 +986,7 @@ int reconfigure_super(struct fs_context *fc) return 0; cancel_readonly: - sb->s_readonly_remount = 0; + sb_end_ro_state_change(sb); return retval; } diff --git a/include/linux/fs.h b/include/linux/fs.h index 133f0640fb24..ede51d60d124 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1242,7 +1242,7 @@ struct super_block { */ atomic_long_t s_fsnotify_connectors; - /* Being remounted read-only */ + /* Read-only state of the superblock is being changed */ int s_readonly_remount; /* per-sb errseq_t for reporting writeback errors via syncfs */
Provide helpers to set and clear sb->s_readonly_remount including appropriate memory barriers. Also use this opportunity to document what the barriers pair with and why they are needed. Suggested-by: Dave Chinner <david@fromorbit.com> Signed-off-by: Jan Kara <jack@suse.cz> --- fs/internal.h | 34 ++++++++++++++++++++++++++++++++++ fs/namespace.c | 10 ++++------ fs/super.c | 17 ++++++----------- include/linux/fs.h | 2 +- 4 files changed, 45 insertions(+), 18 deletions(-)