ceph: skip existing superblocks that are blocklisted when mounting

PATCH Superseded Jeff Layton 3 participants 6 messages Sep 30, 2021 – Oct 12, 2021 2 messages not in the archive Ceph client

lore

Share & tools

lore

Jeff Layton

author

2021-09-30 17:03 UTC #0 lore raw

PATCH v2 1/1 ceph: skip existing superblocks that are blocklisted when mounting

Currently when mounting, we may end up finding an existing superblock that corresponds to a blocklisted MDS client. This means that the new mount ends up being unusable.

If we've found an existing superblock with a client that is already blocklisted, and the client is not configured to recover on its own, fail the match.

While we're in here, also rename "other" to the more conventional "fsc".

Cc: Patrick Donnelly <[email protected]>
Cc: Niels de Vos <[email protected]>
URL: https://bugzilla.redhat.com/show_bug.cgi?id=1901499

Signed-off-by Jeff Layton <[email protected]>

---
 fs/ceph/super.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

fs/ceph/super.c +8 -3

index f517ad9eeb26..a7f1b66a91a7 100644
@@ -1123,16 +1123,16 @@ static int ceph_compare_super(struct super_block *sb, struct fs_context *fc)
 	struct ceph_fs_client *new = fc->s_fs_info;
 	struct ceph_mount_options *fsopt = new->mount_options;
 	struct ceph_options *opt = new->client->options;
-	struct ceph_fs_client *other = ceph_sb_to_client(sb);
+	struct ceph_fs_client *fsc = ceph_sb_to_client(sb);
 
 	dout("ceph_compare_super %p\n", sb);
 
-	if (compare_mount_options(fsopt, opt, other)) {
+	if (compare_mount_options(fsopt, opt, fsc)) {
 		dout("monitor(s)/mount options don't match\n");
 		return 0;
 	}
 	if ((opt->flags & CEPH_OPT_FSID) &&
-	    ceph_fsid_compare(&opt->fsid, &other->client->fsid)) {
+	    ceph_fsid_compare(&opt->fsid, &fsc->client->fsid)) {
 		dout("fsid doesn't match\n");
 		return 0;
 	}

@@ -1140,6 +1140,11 @@ static int ceph_compare_super(struct super_block *sb, struct fs_context *fc)
 		dout("flags differ\n");
 		return 0;
 	}
+	/* Exclude any blocklisted superblocks */
+	if (fsc->blocklisted && !(fsopt->flags & CEPH_MOUNT_OPT_CLEANRECOVER)) {
+		dout("mds client is blocklisted (and CLEANRECOVER is not set)\n");
+		return 0;
+	}
 	return 1;
 }

Signature · 1 line

2.31.1

Nit: This looks a bit weird because fsc is the existing client while fsopt is the new set of mount options. They are guaranteed to match at that point because of compare_mount_options() but it feels better to stick to probing the existing client, e.g.

   if (fsc->blocklisted && !ceph_test_mount_opt(fsc, CLEANRECOVER))

Thanks,

Ilya

Jeff Layton

author

2021-10-05 10:14 UTC #2 lore raw

Quoted 53 lines · Ilya Dryomov

Quoted 44 lines · Jeff Layton

Currently when mounting, we may end up finding an existing superblock that corresponds to a blocklisted MDS client. This means that the new mount ends up being unusable.

If we've found an existing superblock with a client that is already blocklisted, and the client is not configured to recover on its own, fail the match.

While we're in here, also rename "other" to the more conventional "fsc".

Cc: Patrick Donnelly <[email protected]>
Cc: Niels de Vos <[email protected]>
URL: https://bugzilla.redhat.com/show_bug.cgi?id=1901499

Signed-off-by Jeff Layton <[email protected]>

---
 fs/ceph/super.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

fs/ceph/super.c +3 -3

index f517ad9eeb26..a7f1b66a91a7 100644
@@ -1123,16 +1123,16 @@ static int ceph_compare_super(struct super_block *sb, struct fs_context *fc)
        struct ceph_fs_client *new = fc->s_fs_info;
        struct ceph_mount_options *fsopt = new->mount_options;
        struct ceph_options *opt = new->client->options;
-       struct ceph_fs_client *other = ceph_sb_to_client(sb);
+       struct ceph_fs_client *fsc = ceph_sb_to_client(sb);
 
        dout("ceph_compare_super %p\n", sb);
 
-       if (compare_mount_options(fsopt, opt, other)) {
+       if (compare_mount_options(fsopt, opt, fsc)) {
                dout("monitor(s)/mount options don't match\n");
                return 0;
        }
        if ((opt->flags & CEPH_OPT_FSID) &&
-           ceph_fsid_compare(&opt->fsid, &other->client->fsid)) {
+           ceph_fsid_compare(&opt->fsid, &fsc->client->fsid)) {
                dout("fsid doesn't match\n");
                return 0;
        }

fs/ceph/super.c static int ceph_compare_super(struct super_block *sb, struct fs_context *fc) · line 1144

Quoted 5 lines

fs/ceph/super.c +2 -0

@@ -1140,6 +1140,11 @@ static int ceph_compare_super(struct super_block *sb, struct fs_context *fc)
                dout("flags differ\n");
                return 0;
        }
+       /* Exclude any blocklisted superblocks */
+       if (fsc->blocklisted && !(fsopt->flags & CEPH_MOUNT_OPT_CLEANRECOVER)) {

Hi Jeff,

Quoted 7 lines

Nit: This looks a bit weird because fsc is the existing client while fsopt is the new set of mount options. They are guaranteed to match at that point because of compare_mount_options() but it feels better to stick to probing the existing client, e.g.
   if (fsc->blocklisted && !ceph_test_mount_opt(fsc, CLEANRECOVER))

Yeah, that does look cleaner. I went ahead and made that change in the testing branch patch, but I'll skip re-posting it.

Thanks,

Signature · 1 line

Jeff Layton <[email protected]>

A message that is not in the archive (sent off-list or not yet received): CAAM7YA=eFt5HVc5r=5o6A_0iuzKxNf=M3k_7Ctx0eg-DsO6CxA@mail.gmail.com

Jeff Layton

author

2021-10-11 10:48 UTC #4 lore raw

Quoted 22 lines · Zheng

Quoted 19 lines · Jeff Layton
Quoted 1 line · Ilya Dryomov
On Thu, Sep 30, 2021 at 7:03 PM Jeff Layton <[email protected]>
Quoted 2 lines · wrote:
Quoted 2 lines
Currently when mounting, we may end up finding an existing
superblock
Quoted 2 lines
Quoted 2 lines
that corresponds to a blocklisted MDS client. This means that the
new
Quoted 3 lines
Quoted 3 lines
mount ends up being unusable.
If we've found an existing superblock with a client that is
already
Quoted 1 line
Quoted 1 line
blocklisted, and the client is not configured to recover on its
own,
Quoted 4 lines
Quoted 4 lines
fail the match.
While we're in here, also rename "other" to the more conventional

Quoted 7 lines

Quoted 3 lines
"fsc".
Quoted 1 line
Quoted 1 line
Note: we have similar issue for forced umounted superblock

True...

There is a small window of time between when ->umount_begin runs and generic_shutdown_super happens. Between that period, you could match a superblock that's been forcibly umounted and is on the way to being detached from the tree.

I'm a little less concerned about that because the time window should be pretty short, and someone would need to try to make a new mount while "umount -f" is still running. I don't think we can fully prevent that anyway, as there is some raciness involved (your mount might match the thing just before umount_begin runs).

I'm inclined not to worry about that case, unless we have some reports of people hitting that problem.

Thoughts?

Quoted 64 lines

Quoted 64 lines
Quoted 8 lines
Quoted 8 lines
Cc: Patrick Donnelly <[email protected]>
Cc: Niels de Vos <[email protected]>
URL: https://bugzilla.redhat.com/show_bug.cgi?id=1901499
Signed-off-by Jeff Layton <[email protected]>
---
fs/ceph/super.c | 11 ++++++++---
  1 file changed, 8 insertions(+), 3 deletions(-)
Quoted 5 lines
fs/ceph/super.c static int ceph_compare_super(struct
Quoted 5 lines
fs/ceph/super.c +0 -0
index f517ad9eeb26..a7f1b66a91a7 100644
@@ -1123,16 +1123,16 @@ static int ceph_compare_super(struct
super_block *sb, struct fs_context *fc)
Quoted 8 lines
Quoted 8 lines
fs/ceph/super.c +1 -1
          struct ceph_fs_client *new = fc->s_fs_info;
          struct ceph_mount_options *fsopt = new->mount_options;
          struct ceph_options *opt = new->client->options;
-       struct ceph_fs_client *other = ceph_sb_to_client(sb);
+       struct ceph_fs_client *fsc = ceph_sb_to_client(sb);
 
          dout("ceph_compare_super %p\n", sb);
 
Quoted 13 lines
fs/ceph/super.c static int ceph_compare_super(struct
Quoted 13 lines
fs/ceph/super.c +1 -1
-       if (compare_mount_options(fsopt, opt, other)) {
+       if (compare_mount_options(fsopt, opt, fsc)) {
                  dout("monitor(s)/mount options don't match\n");
                  return 0;
          }
if ((opt->flags & CEPH_OPT_FSID) &&
- ceph_fsid_compare(&opt->fsid, &other->client->fsid)) {
+ ceph_fsid_compare(&opt->fsid, &fsc->client->fsid)) { dout("fsid doesn't match\n");
return 0;
}
fs/ceph/super.c +0 -0
@@ -1140,6 +1140,11 @@ static int ceph_compare_super(struct
super_block *sb, struct fs_context *fc)
Quoted 5 lines
fs/ceph/super.c static int ceph_compare_super(struct super_block *sb, struct fs_context *fc)
Quoted 5 lines
fs/ceph/super.c +2 -0
                  dout("flags differ\n");
                  return 0;
          }
+       /* Exclude any blocklisted superblocks */
+       if (fsc->blocklisted && !(fsopt->flags &
CEPH_MOUNT_OPT_CLEANRECOVER)) {
Quoted 7 lines
Hi Jeff,
Nit: This looks a bit weird because fsc is the existing client while
fsopt is the new set of mount options. They are guaranteed to match
at
Quoted 7 lines
that point because of compare_mount_options() but it feels better to
stick to probing the existing client, e.g.
if (fsc->blocklisted && !ceph_test_mount_opt(fsc, CLEANRECOVER))
Yeah, that does look cleaner. I went ahead and made that change in the
testing branch patch, but I'll skip re-posting it.
Thanks,

Signature · 1 line

Jeff Layton <[email protected]>

A message that is not in the archive (sent off-list or not yet received): CAAM7YAmE8hs8sRtZgz99mb_9M2W84BBZU5u2n-BZF2SfN9dU7Q@mail.gmail.com

Jeff Layton

author

2021-10-12 13:48 UTC #6 lore raw

Quoted 49 lines · Zheng

Quoted 35 lines · Jeff Layton
Quoted 3 lines · Zheng
On Tue, Oct 5, 2021 at 6:17 PM Jeff Layton <[email protected]>
Quoted 10 lines · wrote:
Quoted 10 lines · Ilya Dryomov
Quoted 2 lines
On Thu, Sep 30, 2021 at 7:03 PM Jeff Layton
<[email protected]>
Quoted 2 lines · wrote:
Quoted 2 lines
Currently when mounting, we may end up finding an existing
superblock
Quoted 3 lines
Quoted 3 lines
that corresponds to a blocklisted MDS client. This means that
the
Quoted 6 lines
Quoted 6 lines
new
Quoted 3 lines
Quoted 3 lines
mount ends up being unusable.
If we've found an existing superblock with a client that is
already
Quoted 1 line
Quoted 1 line
blocklisted, and the client is not configured to recover on
its
Quoted 5 lines
Quoted 5 lines
own,
Quoted 4 lines
Quoted 4 lines
fail the match.
While we're in here, also rename "other" to the more conventional
Quoted 7 lines
Quoted 3 lines
"fsc".
Quoted 1 line
Quoted 1 line
Note: we have similar issue for forced umounted superblock
Quoted 8 lines
True...
There is a small window of time between when ->umount_begin runs and generic_shutdown_super happens. Between that period, you could match a
superblock that's been forcibly umounted and is on the way to being detached from the tree.

Quoted 3 lines

I mean set fsc->mount_state to CEPH_MOUNT_SHUTDOWN, but failed to fully shutdown (still referenced) the superblock

Good point. We could have a superblock that is still extant but that was forcibly unmounted and detached. I'll send a v3 patch that incorporates that case as well.

Thanks,

Signature · 1 line

Jeff Layton <[email protected]>

Xiubo Li

Reviewed-by on v2 1/1

2021-10-08 08:37 UTC #7 lore raw

In reply to Jeff Layton

Quoted 44 lines · Jeff Layton

Currently when mounting, we may end up finding an existing superblock that corresponds to a blocklisted MDS client. This means that the new mount ends up being unusable.
If we've found an existing superblock with a client that is already blocklisted, and the client is not configured to recover on its own, fail the match.
While we're in here, also rename "other" to the more conventional "fsc".
Cc: Patrick Donnelly <[email protected]>
Cc: Niels de Vos <[email protected]>
URL: https://bugzilla.redhat.com/show_bug.cgi?id=1901499
Signed-off-by Jeff Layton <[email protected]>
---
  fs/ceph/super.c | 11 ++++++++---
  1 file changed, 8 insertions(+), 3 deletions(-)
fs/ceph/super.c +3 -3
index f517ad9eeb26..a7f1b66a91a7 100644
@@ -1123,16 +1123,16 @@ static int ceph_compare_super(struct super_block *sb, struct fs_context *fc)
  	struct ceph_fs_client *new = fc->s_fs_info;
  	struct ceph_mount_options *fsopt = new->mount_options;
  	struct ceph_options *opt = new->client->options;
-	struct ceph_fs_client *other = ceph_sb_to_client(sb);
+	struct ceph_fs_client *fsc = ceph_sb_to_client(sb);
 
  	dout("ceph_compare_super %p\n", sb);
 
-	if (compare_mount_options(fsopt, opt, other)) {
+	if (compare_mount_options(fsopt, opt, fsc)) {
  		dout("monitor(s)/mount options don't match\n");
  		return 0;
  	}
  	if ((opt->flags & CEPH_OPT_FSID) &&
-	    ceph_fsid_compare(&opt->fsid, &other->client->fsid)) {
+	    ceph_fsid_compare(&opt->fsid, &fsc->client->fsid)) {
  		dout("fsid doesn't match\n");
  		return 0;
  	}
@@ -1140,6 +1140,11 @@ static int ceph_compare_super(struct super_block *sb, struct fs_context *fc)
  		dout("flags differ\n");

fs/ceph/super.c static int ceph_compare_super(struct super_block *sb, struct fs_context *fc) · line 1150

Quoted 10 lines

fs/ceph/super.c +5 -0

  		return 0;
  	}
+	/* Exclude any blocklisted superblocks */
+	if (fsc->blocklisted && !(fsopt->flags & CEPH_MOUNT_OPT_CLEANRECOVER)) {
+		dout("mds client is blocklisted (and CLEANRECOVER is not set)\n");
+		return 0;
+	}
  	return 1;
  }

LGTM.

Reviewed-by Xiubo Li <[email protected]>