| Message ID | 20171129232356.28296-4-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed 29-11-17 15:23:48, Luis R. Rodriguez wrote: > The question of whether or not a superblock is frozen needs to be > augmented in the future to account for differences between a user > initiated freeze and a kernel initiated freeze done automatically > on behalf of the kernel. > > Provide helpers so that these can be used instead so that we don't > have to expand checks later in these same call sites as we expand > the definition of a frozen superblock. > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> So helpers are fine but... > +/** > + * sb_is_frozen_by_user - is superblock frozen by a user call > + * @sb: the super to check > + * > + * Returns true if the super freeze was initiated by userspace, for instance, > + * an ioctl call. > + */ > +static inline bool sb_is_frozen_by_user(struct super_block *sb) > +{ > + return sb->s_writers.frozen == SB_FREEZE_COMPLETE; > +} ... I dislike the _by_user() suffix as there may be different places that call freeze_super() (e.g. device mapper does this during some operations). Clearly we need to distinguish "by system suspend" and "the other" cases. So please make this clear in the naming. In fact, what might be a cleaner solution is to introduce a 'freeze_count' for superblock freezing (we already do have this for block devices). Then you don't need to differentiate these two cases - but you'd still need to properly handle cleanup if freezing of all superblocks fails in the middle. So I'm not 100% this works out nicely in the end. But it's certainly worth a consideration. Honza
On Thu, Nov 30, 2017 at 06:13:10PM +0100, Jan Kara wrote: > ... I dislike the _by_user() suffix as there may be different places that > call freeze_super() (e.g. device mapper does this during some operations). > Clearly we need to distinguish "by system suspend" and "the other" cases. > So please make this clear in the naming. Ah. How about sb_frozen_by_cb() ? > In fact, what might be a cleaner solution is to introduce a 'freeze_count' > for superblock freezing (we already do have this for block devices). Then > you don't need to differentiate these two cases - but you'd still need to > properly handle cleanup if freezing of all superblocks fails in the middle. > So I'm not 100% this works out nicely in the end. But it's certainly worth > a consideration. Ah, there are three important reasons for doing it the way I did it which are easy to miss, unless you read the commit log message very carefully. 0) The ioctl interface causes a failure to be sent back to userspace if you issue two consecutive freezes, or two thaws. Ie, once a filesystem is frozen, a secondary call will result in an error. Likewise for thaw. 1) The new iterate supers stuff I added bail on the first error and return that error. If we kept the ioctl() interface error scheme we'd be erroring out if on suspend if userspace had already frozen a filesystem. Clearly that'd be silly so we need to distinguish between the automatic kernel freezing and the old userspace ioctl initiated interface, so that we can keep the old behaviour but allow in-kernel auto freeze on suspend to work properly. 2) If we fail to suspend we need to then thaw up all filesystems. The order in which we try to freeze is in reverse order on the super_block list. If we fail though we iterate in proper order on the super_block list and thaw. If you had two filesystems this means that if a failure happened on freezing the first filesystem, we'd first thaw the other filesystem -- and because of 0) if we don't distinguish between the ioctl interface or auto freezing, we'd also fail on thaw'ing given the other superblock wouldn't have been frozen. So we need to keep two separate approaches. The count stuff would not suffice to distinguish origin of source for freeze call. Come to think of it, I think I forgot to avoid thaw if the freeze was ioctl initiated.. thaw_unlocked(bool cb_call) { if (sb_frozen_by_cb(sb) && !cb_call) return 0; /* skip as the user wanted to keep this fs frozen */ ... } Even though the kernel auto call is new I think we need to keep ioctl initiated frozen filesystems frozen to not break old userspace assumptions. So, keeping all this in mind, does a count method still suffice? Luis
On Thu 30-11-17 20:05:48, Luis R. Rodriguez wrote: > On Thu, Nov 30, 2017 at 06:13:10PM +0100, Jan Kara wrote: > > ... I dislike the _by_user() suffix as there may be different places that > > call freeze_super() (e.g. device mapper does this during some operations). > > Clearly we need to distinguish "by system suspend" and "the other" cases. > > So please make this clear in the naming. > > Ah. How about sb_frozen_by_cb() ? And what does 'cb' stand for? :) > > In fact, what might be a cleaner solution is to introduce a 'freeze_count' > > for superblock freezing (we already do have this for block devices). Then > > you don't need to differentiate these two cases - but you'd still need to > > properly handle cleanup if freezing of all superblocks fails in the middle. > > So I'm not 100% this works out nicely in the end. But it's certainly worth > > a consideration. > > Ah, there are three important reasons for doing it the way I did it which are > easy to miss, unless you read the commit log message very carefully. > > 0) The ioctl interface causes a failure to be sent back to userspace if > you issue two consecutive freezes, or two thaws. Ie, once a filesystem is > frozen, a secondary call will result in an error. Likewise for thaw. Yep. But also note that there's *another* interface to filesystem freezing which behaves differently - freeze_bdev() (used internally by dm). That interface uses the counter and freezing of already frozen device succeeds. IOW it is a mess. We cannot change the behavior of the ioctl but we could still provide an in-kernel interface to freeze_super() with the same semantics as freeze_bdev() which might be easier to use by suspend - maybe we could call it 'exclusive' (for the current freeze_super() semantics) and 'non-exclusive' (for the freeze_bdev() semantics) since this is very much like O_EXCL open of block devices... > 1) The new iterate supers stuff I added bail on the first error and return that > error. If we kept the ioctl() interface error scheme we'd be erroring out > if on suspend if userspace had already frozen a filesystem. Clearly that'd > be silly so we need to distinguish between the automatic kernel freezing > and the old userspace ioctl initiated interface, so that we can keep the > old behaviour but allow in-kernel auto freeze on suspend to work properly. This would work fine with the non-exclusive semantics I believe. > 2) If we fail to suspend we need to then thaw up all filesystems. The order > in which we try to freeze is in reverse order on the super_block list. If we > fail though we iterate in proper order on the super_block list and thaw. If > you had two filesystems this means that if a failure happened on freezing > the first filesystem, we'd first thaw the other filesystem -- and because of > 0) if we don't distinguish between the ioctl interface or auto freezing, we'd > also fail on thaw'ing given the other superblock wouldn't have been frozen. > > So we need to keep two separate approaches. The count stuff would not suffice > to distinguish origin of source for freeze call. > > Come to think of it, I think I forgot to avoid thaw if the freeze was ioctl > initiated.. > > thaw_unlocked(bool cb_call) > { > if (sb_frozen_by_cb(sb) && !cb_call) > return 0; /* skip as the user wanted to keep this fs frozen */ > ... > } > > Even though the kernel auto call is new I think we need to keep ioctl initiated > frozen filesystems frozen to not break old userspace assumptions. > > So, keeping all this in mind, does a count method still suffice? The count method would need a different error recovery method - i.e. if you fail freezing filesystems somewhere in the middle of iteration through superblock list, you'd need to iterate from that point on to the superblock where you've started. This is somewhat more complicated than your approach but it seems cleaner to me: 1) Function freezing all superblocks either succeeds and all superblocks are frozen or fails and no superblocks are (additionally) frozen. 2) It is not that normal users + one special user (who owns the "flag" in the superblock in form of a special freeze state) setup. We'd simply have exclusive and non-exclusive users of superblock freezing and there can be arbitrary numbers of them. Honza
On Thu, Nov 30, 2017 at 06:13:10PM +0100, Jan Kara wrote: > On Wed 29-11-17 15:23:48, Luis R. Rodriguez wrote: > > The question of whether or not a superblock is frozen needs to be > > augmented in the future to account for differences between a user > > initiated freeze and a kernel initiated freeze done automatically > > on behalf of the kernel. > > > > Provide helpers so that these can be used instead so that we don't > > have to expand checks later in these same call sites as we expand > > the definition of a frozen superblock. > > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > > So helpers are fine but... > > > +/** > > + * sb_is_frozen_by_user - is superblock frozen by a user call > > + * @sb: the super to check > > + * > > + * Returns true if the super freeze was initiated by userspace, for instance, > > + * an ioctl call. > > + */ > > +static inline bool sb_is_frozen_by_user(struct super_block *sb) > > +{ > > + return sb->s_writers.frozen == SB_FREEZE_COMPLETE; > > +} > > ... I dislike the _by_user() suffix as there may be different places that > call freeze_super() (e.g. device mapper does this during some operations). > Clearly we need to distinguish "by system suspend" and "the other" cases. > So please make this clear in the naming. > > In fact, what might be a cleaner solution is to introduce a 'freeze_count' > for superblock freezing (we already do have this for block devices). Then > you don't need to differentiate these two cases - but you'd still need to > properly handle cleanup if freezing of all superblocks fails in the middle. > So I'm not 100% this works out nicely in the end. But it's certainly worth > a consideration. Seems reasonable. Luis
diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c index 2d593201cf7a..090b8cd4551a 100644 --- a/fs/ext4/ext4_jbd2.c +++ b/fs/ext4/ext4_jbd2.c @@ -50,7 +50,7 @@ static int ext4_journal_check_start(struct super_block *sb) if (sb_rdonly(sb)) return -EROFS; - WARN_ON(sb->s_writers.frozen == SB_FREEZE_COMPLETE); + WARN_ON(sb_is_frozen(sb)); journal = EXT4_SB(sb)->s_journal; /* * Special case here: if the journal has aborted behind our diff --git a/fs/super.c b/fs/super.c index cecc279beecd..e8f5a7139b8f 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1392,7 +1392,7 @@ static int freeze_locked_super(struct super_block *sb) { int ret; - if (sb->s_writers.frozen != SB_UNFROZEN) + if (!sb_is_unfrozen(sb)) return -EBUSY; if (!(sb->s_flags & SB_BORN)) @@ -1498,7 +1498,7 @@ static int thaw_locked_super(struct super_block *sb) { int error; - if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) + if (!sb_is_frozen(sb)) return -EINVAL; if (sb_rdonly(sb)) { diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c index a87f657f59c9..b1180c26d902 100644 --- a/fs/xfs/xfs_trans.c +++ b/fs/xfs/xfs_trans.c @@ -241,7 +241,7 @@ xfs_trans_alloc( if (!(flags & XFS_TRANS_NO_WRITECOUNT)) sb_start_intwrite(mp->m_super); - WARN_ON(mp->m_super->s_writers.frozen == SB_FREEZE_COMPLETE); + WARN_ON(sb_is_frozen(mp->m_super)); atomic_inc(&mp->m_active_trans); tp = kmem_zone_zalloc(xfs_trans_zone, diff --git a/include/linux/fs.h b/include/linux/fs.h index 511fbaabf624..1e10239c1d3b 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1589,6 +1589,39 @@ static inline void sb_start_intwrite(struct super_block *sb) __sb_start_write(sb, SB_FREEZE_FS, true); } +/** + * sb_is_frozen_by_user - is superblock frozen by a user call + * @sb: the super to check + * + * Returns true if the super freeze was initiated by userspace, for instance, + * an ioctl call. + */ +static inline bool sb_is_frozen_by_user(struct super_block *sb) +{ + return sb->s_writers.frozen == SB_FREEZE_COMPLETE; +} + +/** + * sb_is_frozen - is superblock frozen + * @sb: the super to check + * + * Returns true if the super is frozen. + */ +static inline bool sb_is_frozen(struct super_block *sb) +{ + return sb_is_frozen_by_user(sb); +} + +/** + * sb_is_unfrozen - is superblock unfrozen + * @sb: the super to check + * + * Returns true if the super is unfrozen. + */ +static inline bool sb_is_unfrozen(struct super_block *sb) +{ + return sb->s_writers.frozen == SB_UNFROZEN; +} extern bool inode_owner_or_capable(const struct inode *inode);
The question of whether or not a superblock is frozen needs to be augmented in the future to account for differences between a user initiated freeze and a kernel initiated freeze done automatically on behalf of the kernel. Provide helpers so that these can be used instead so that we don't have to expand checks later in these same call sites as we expand the definition of a frozen superblock. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- fs/ext4/ext4_jbd2.c | 2 +- fs/super.c | 4 ++-- fs/xfs/xfs_trans.c | 2 +- include/linux/fs.h | 33 +++++++++++++++++++++++++++++++++ 4 files changed, 37 insertions(+), 4 deletions(-)