* [PATCH v2] ceph: skip existing superblocks that are blocklisted when mounting
@ 2021-09-30 17:03 Jeff Layton
2021-10-05 8:00 ` Ilya Dryomov
2021-10-08 8:37 ` Xiubo Li
0 siblings, 2 replies; 6+ messages in thread
From: Jeff Layton @ 2021-09-30 17:03 UTC (permalink / raw)
To: ceph-devel; +Cc: idryomov, Patrick Donnelly, Niels de Vos
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 <pdonnell@redhat.com>
Cc: Niels de Vos <ndevos@redhat.com>
URL: https://bugzilla.redhat.com/show_bug.cgi?id=1901499
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
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)) {
+ dout("mds client is blocklisted (and CLEANRECOVER is not set)\n");
+ return 0;
+ }
return 1;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ceph: skip existing superblocks that are blocklisted when mounting
2021-09-30 17:03 [PATCH v2] ceph: skip existing superblocks that are blocklisted when mounting Jeff Layton
@ 2021-10-05 8:00 ` Ilya Dryomov
2021-10-05 10:14 ` Jeff Layton
2021-10-08 8:37 ` Xiubo Li
1 sibling, 1 reply; 6+ messages in thread
From: Ilya Dryomov @ 2021-10-05 8:00 UTC (permalink / raw)
To: Jeff Layton; +Cc: Ceph Development, Patrick Donnelly, Niels de Vos
On Thu, Sep 30, 2021 at 7:03 PM Jeff Layton <jlayton@kernel.org> 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".
>
> Cc: Patrick Donnelly <pdonnell@redhat.com>
> Cc: Niels de Vos <ndevos@redhat.com>
> URL: https://bugzilla.redhat.com/show_bug.cgi?id=1901499
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> 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))
Thanks,
Ilya
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ceph: skip existing superblocks that are blocklisted when mounting
2021-10-05 8:00 ` Ilya Dryomov
@ 2021-10-05 10:14 ` Jeff Layton
[not found] ` <CAAM7YA=eFt5HVc5r=5o6A_0iuzKxNf=M3k_7Ctx0eg-DsO6CxA@mail.gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2021-10-05 10:14 UTC (permalink / raw)
To: Ilya Dryomov; +Cc: Ceph Development, Patrick Donnelly, Niels de Vos
On Tue, 2021-10-05 at 10:00 +0200, Ilya Dryomov wrote:
> On Thu, Sep 30, 2021 at 7:03 PM Jeff Layton <jlayton@kernel.org> 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".
> >
> > Cc: Patrick Donnelly <pdonnell@redhat.com>
> > Cc: Niels de Vos <ndevos@redhat.com>
> > URL: https://bugzilla.redhat.com/show_bug.cgi?id=1901499
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > 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@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ceph: skip existing superblocks that are blocklisted when mounting
2021-09-30 17:03 [PATCH v2] ceph: skip existing superblocks that are blocklisted when mounting Jeff Layton
2021-10-05 8:00 ` Ilya Dryomov
@ 2021-10-08 8:37 ` Xiubo Li
1 sibling, 0 replies; 6+ messages in thread
From: Xiubo Li @ 2021-10-08 8:37 UTC (permalink / raw)
To: Jeff Layton, ceph-devel; +Cc: idryomov, Patrick Donnelly, Niels de Vos
On 10/1/21 1:03 AM, Jeff Layton 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".
>
> Cc: Patrick Donnelly <pdonnell@redhat.com>
> Cc: Niels de Vos <ndevos@redhat.com>
> URL: https://bugzilla.redhat.com/show_bug.cgi?id=1901499
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> 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)) {
> + dout("mds client is blocklisted (and CLEANRECOVER is not set)\n");
> + return 0;
> + }
> return 1;
> }
>
LGTM.
Reviewed-by: Xiubo Li <xiubli@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ceph: skip existing superblocks that are blocklisted when mounting
[not found] ` <CAAM7YA=eFt5HVc5r=5o6A_0iuzKxNf=M3k_7Ctx0eg-DsO6CxA@mail.gmail.com>
@ 2021-10-11 10:48 ` Jeff Layton
[not found] ` <CAAM7YAmE8hs8sRtZgz99mb_9M2W84BBZU5u2n-BZF2SfN9dU7Q@mail.gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2021-10-11 10:48 UTC (permalink / raw)
To: Yan, Zheng; +Cc: Ilya Dryomov, Ceph Development, Patrick Donnelly, Niels de Vos
On Mon, 2021-10-11 at 11:37 +0800, Yan, Zheng wrote:
>
>
> On Tue, Oct 5, 2021 at 6:17 PM Jeff Layton <jlayton@kernel.org> 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@kernel.org>
> > 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@redhat.com>
> > > > Cc: Niels de Vos <ndevos@redhat.com>
> > > > URL: https://bugzilla.redhat.com/show_bug.cgi?id=1901499
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > > 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@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ceph: skip existing superblocks that are blocklisted when mounting
[not found] ` <CAAM7YAmE8hs8sRtZgz99mb_9M2W84BBZU5u2n-BZF2SfN9dU7Q@mail.gmail.com>
@ 2021-10-12 13:48 ` Jeff Layton
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Layton @ 2021-10-12 13:48 UTC (permalink / raw)
To: Yan, Zheng; +Cc: Ilya Dryomov, Ceph Development, Patrick Donnelly, Niels de Vos
On Tue, 2021-10-12 at 15:09 +0800, Yan, Zheng wrote:
>
>
> On Mon, Oct 11, 2021 at 6:48 PM Jeff Layton <jlayton@kernel.org>
> wrote:
> > On Mon, 2021-10-11 at 11:37 +0800, Yan, Zheng wrote:
> > >
> > >
> > > On Tue, Oct 5, 2021 at 6:17 PM Jeff Layton <jlayton@kernel.org>
> > 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@kernel.org>
> > > > 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 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,
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-12 13:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-30 17:03 [PATCH v2] ceph: skip existing superblocks that are blocklisted when mounting Jeff Layton
2021-10-05 8:00 ` Ilya Dryomov
2021-10-05 10:14 ` Jeff Layton
[not found] ` <CAAM7YA=eFt5HVc5r=5o6A_0iuzKxNf=M3k_7Ctx0eg-DsO6CxA@mail.gmail.com>
2021-10-11 10:48 ` Jeff Layton
[not found] ` <CAAM7YAmE8hs8sRtZgz99mb_9M2W84BBZU5u2n-BZF2SfN9dU7Q@mail.gmail.com>
2021-10-12 13:48 ` Jeff Layton
2021-10-08 8:37 ` Xiubo Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.