On Mon, 2021-10-11 at 11:37 +0800, Yan, Zheng wrote: > > > On Tue, Oct 5, 2021 at 6:17 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > On Tue, 2021-10-05 at 10:00 +0200, Ilya Dryomov wrote: > > > On Thu, Sep 30, 2021 at 7:03 PM Jeff Layton <jlayton@xxxxxxxxxx> > > wrote: > > > > > > > > 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". > > > > > > > > > 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? > > > > > > Cc: Patrick Donnelly <pdonnell@xxxxxxxxxx> > > > > Cc: Niels de Vos <ndevos@xxxxxxxxxx> > > > > URL: https://bugzilla.redhat.com/show_bug.cgi?id=1901499 > > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> > > > > --- > > > > fs/ceph/super.c | 11 ++++++++--- > > > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/fs/ceph/super.c b/fs/ceph/super.c > > > > index f517ad9eeb26..a7f1b66a91a7 100644 > > > > --- a/fs/ceph/super.c > > > > +++ b/fs/ceph/super.c > > > > @@ -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)) { > > > > > > 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 > > > 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, -- Jeff Layton <jlayton@xxxxxxxxxx>
