2023-05-08 01:31:05

by Luis Chamberlain

[permalink] [raw]
Subject: [PATCH 2/6] fs: add frozen sb state helpers

Provide helpers so that we can check a superblock frozen state.
This will make subsequent changes easier to read. This makes
no functional changes.

Reviewed-by: Jan Kara <[email protected]>
Signed-off-by: Luis Chamberlain <[email protected]>
---
fs/ext4/ext4_jbd2.c | 2 +-
fs/gfs2/sys.c | 2 +-
fs/quota/quota.c | 4 ++--
fs/super.c | 6 +++---
fs/xfs/xfs_trans.c | 3 +--
include/linux/fs.h | 22 ++++++++++++++++++++++
6 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
index 77f318ec8abb..ef441f15053b 100644
--- a/fs/ext4/ext4_jbd2.c
+++ b/fs/ext4/ext4_jbd2.c
@@ -72,7 +72,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/gfs2/sys.c b/fs/gfs2/sys.c
index cbb71c3520c0..e80c827acd09 100644
--- a/fs/gfs2/sys.c
+++ b/fs/gfs2/sys.c
@@ -148,7 +148,7 @@ static ssize_t uuid_show(struct gfs2_sbd *sdp, char *buf)
static ssize_t freeze_show(struct gfs2_sbd *sdp, char *buf)
{
struct super_block *sb = sdp->sd_vfs;
- int frozen = (sb->s_writers.frozen == SB_UNFROZEN) ? 0 : 1;
+ int frozen = sb_is_unfrozen(sb) ? 0 : 1;

return snprintf(buf, PAGE_SIZE, "%d\n", frozen);
}
diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 052f143e2e0e..66ea23e15d93 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -890,13 +890,13 @@ static struct super_block *quotactl_block(const char __user *special, int cmd)
sb = user_get_super(dev, excl);
if (!sb)
return ERR_PTR(-ENODEV);
- if (thawed && sb->s_writers.frozen != SB_UNFROZEN) {
+ if (thawed && !sb_is_unfrozen(sb)) {
if (excl)
up_write(&sb->s_umount);
else
up_read(&sb->s_umount);
wait_event(sb->s_writers.wait_unfrozen,
- sb->s_writers.frozen == SB_UNFROZEN);
+ sb_is_unfrozen(sb));
put_super(sb);
goto retry;
}
diff --git a/fs/super.c b/fs/super.c
index 0e9d48846684..46c6475fc765 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -905,7 +905,7 @@ int reconfigure_super(struct fs_context *fc)

if (fc->sb_flags_mask & ~MS_RMT_MASK)
return -EINVAL;
- if (sb->s_writers.frozen != SB_UNFROZEN)
+ if (!(sb_is_unfrozen(sb)))
return -EBUSY;

retval = security_sb_remount(sb, fc->security);
@@ -929,7 +929,7 @@ int reconfigure_super(struct fs_context *fc)
down_write(&sb->s_umount);
if (!sb->s_root)
return 0;
- if (sb->s_writers.frozen != SB_UNFROZEN)
+ if (!sb_is_unfrozen(sb))
return -EBUSY;
remount_ro = !sb_rdonly(sb);
}
@@ -1673,7 +1673,7 @@ int freeze_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))
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 8afc0c080861..26caeafc572f 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -267,8 +267,7 @@ xfs_trans_alloc(
* Zero-reservation ("empty") transactions can't modify anything, so
* they're allowed to run while we're frozen.
*/
- WARN_ON(resp->tr_logres > 0 &&
- mp->m_super->s_writers.frozen == SB_FREEZE_COMPLETE);
+ WARN_ON(resp->tr_logres > 0 && sb_is_frozen(mp->m_super));
ASSERT(!(flags & XFS_TRANS_RES_FDBLKS) ||
xfs_has_lazysbcount(mp));

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 21a981680856..90b5bdc4071a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1621,6 +1621,28 @@ static inline bool sb_start_intwrite_trylock(struct super_block *sb)
return __sb_start_write_trylock(sb, SB_FREEZE_FS);
}

+/**
+ * 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->s_writers.frozen == SB_FREEZE_COMPLETE;
+}
+
+/**
+ * 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;
+}
+
bool inode_owner_or_capable(struct mnt_idmap *idmap,
const struct inode *inode);

--
2.39.2


2023-05-25 12:30:12

by Jan Kara

[permalink] [raw]
Subject: Re: [PATCH 2/6] fs: add frozen sb state helpers

On Sun 07-05-23 18:17:13, Luis Chamberlain wrote:
> Provide helpers so that we can check a superblock frozen state.
> This will make subsequent changes easier to read. This makes
> no functional changes.
>
> Reviewed-by: Jan Kara <[email protected]>
> Signed-off-by: Luis Chamberlain <[email protected]>

Just noticed one nit...

> diff --git a/fs/super.c b/fs/super.c
> index 0e9d48846684..46c6475fc765 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -905,7 +905,7 @@ int reconfigure_super(struct fs_context *fc)
>
> if (fc->sb_flags_mask & ~MS_RMT_MASK)
> return -EINVAL;
> - if (sb->s_writers.frozen != SB_UNFROZEN)
> + if (!(sb_is_unfrozen(sb)))
^ unneeded parenthesis here

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

2023-06-08 05:34:09

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 2/6] fs: add frozen sb state helpers

On Sun, May 07, 2023 at 06:17:13PM -0700, Luis Chamberlain wrote:
> Provide helpers so that we can check a superblock frozen state.
> This will make subsequent changes easier to read. This makes
> no functional changes.

I'll look at the further changes, but having frozen/unfrozen helpers
that sound binary while we have 4 shades of gray seems rather confusing
to me.


2023-06-08 15:31:11

by Darrick J. Wong

[permalink] [raw]
Subject: Re: [PATCH 2/6] fs: add frozen sb state helpers

On Wed, Jun 07, 2023 at 10:05:46PM -0700, Christoph Hellwig wrote:
> On Sun, May 07, 2023 at 06:17:13PM -0700, Luis Chamberlain wrote:
> > Provide helpers so that we can check a superblock frozen state.
> > This will make subsequent changes easier to read. This makes
> > no functional changes.
>
> I'll look at the further changes, but having frozen/unfrozen helpers
> that sound binary while we have 4 shades of gray seems rather confusing
> to me.

That was my first reaction too.

Then it occurred to me that *some* people might still be clued into that
subtlety if they happened to ask themselves why there are predicates for
_is_frozen and _is_unfrozen.

But in the end I think I decided that an enum isn't subtle like that at
all and clearly forgot to reply with that.

--D