ceph-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ceph: enable async dirops by default
@ 2021-08-09 16:44 Jeff Layton
  2021-09-01 16:54 ` Jeff Layton
  0 siblings, 1 reply; 10+ messages in thread
From: Jeff Layton @ 2021-08-09 16:44 UTC (permalink / raw)
  To: ceph-devel; +Cc: idryomov, Patrick Donnelly

Async dirops have been supported in mainline kernels for quite some time
now, and we've recently (as of June) started doing regular testing in
teuthology with '-o nowsync'. So far, that hasn't uncovered any issues,
so I think the time is right to flip the default for this option.

Enable async dirops by default, and change /proc/mounts to show "wsync"
when they are disabled rather than "nowsync" when they are enabled.

Cc: Patrick Donnelly <pdonnell@redhat.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/super.c | 4 ++--
 fs/ceph/super.h | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 609ffc8c2d78..f517ad9eeb26 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -698,8 +698,8 @@ static int ceph_show_options(struct seq_file *m, struct dentry *root)
 	if (fsopt->flags & CEPH_MOUNT_OPT_CLEANRECOVER)
 		seq_show_option(m, "recover_session", "clean");
 
-	if (fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS)
-		seq_puts(m, ",nowsync");
+	if (!(fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS))
+		seq_puts(m, ",wsync");
 
 	if (fsopt->wsize != CEPH_MAX_WRITE_SIZE)
 		seq_printf(m, ",wsize=%u", fsopt->wsize);
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index 389b45ac291b..0bc36cf4c683 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -48,7 +48,8 @@
 
 #define CEPH_MOUNT_OPT_DEFAULT			\
 	(CEPH_MOUNT_OPT_DCACHE |		\
-	 CEPH_MOUNT_OPT_NOCOPYFROM)
+	 CEPH_MOUNT_OPT_NOCOPYFROM |		\
+	 CEPH_MOUNT_OPT_ASYNC_DIROPS)
 
 #define ceph_set_mount_opt(fsc, opt) \
 	(fsc)->mount_options->flags |= CEPH_MOUNT_OPT_##opt
-- 
2.31.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ceph: enable async dirops by default
  2021-08-09 16:44 [PATCH] ceph: enable async dirops by default Jeff Layton
@ 2021-09-01 16:54 ` Jeff Layton
  2021-09-02 15:52   ` Ilya Dryomov
  2021-09-07 21:07   ` Patrick Donnelly
  0 siblings, 2 replies; 10+ messages in thread
From: Jeff Layton @ 2021-09-01 16:54 UTC (permalink / raw)
  To: ceph-devel, Ilya Dryomov; +Cc: Patrick Donnelly

On Mon, 2021-08-09 at 12:44 -0400, Jeff Layton wrote:
> Async dirops have been supported in mainline kernels for quite some time
> now, and we've recently (as of June) started doing regular testing in
> teuthology with '-o nowsync'. So far, that hasn't uncovered any issues,
> so I think the time is right to flip the default for this option.
> 
> Enable async dirops by default, and change /proc/mounts to show "wsync"
> when they are disabled rather than "nowsync" when they are enabled.
> 
> Cc: Patrick Donnelly <pdonnell@redhat.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/super.c | 4 ++--
>  fs/ceph/super.h | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 609ffc8c2d78..f517ad9eeb26 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -698,8 +698,8 @@ static int ceph_show_options(struct seq_file *m, struct dentry *root)
>  	if (fsopt->flags & CEPH_MOUNT_OPT_CLEANRECOVER)
>  		seq_show_option(m, "recover_session", "clean");
>  
> -	if (fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS)
> -		seq_puts(m, ",nowsync");
> +	if (!(fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS))
> +		seq_puts(m, ",wsync");
>  
>  	if (fsopt->wsize != CEPH_MAX_WRITE_SIZE)
>  		seq_printf(m, ",wsize=%u", fsopt->wsize);
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index 389b45ac291b..0bc36cf4c683 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -48,7 +48,8 @@
>  
>  #define CEPH_MOUNT_OPT_DEFAULT			\
>  	(CEPH_MOUNT_OPT_DCACHE |		\
> -	 CEPH_MOUNT_OPT_NOCOPYFROM)
> +	 CEPH_MOUNT_OPT_NOCOPYFROM |		\
> +	 CEPH_MOUNT_OPT_ASYNC_DIROPS)
>  
>  #define ceph_set_mount_opt(fsc, opt) \
>  	(fsc)->mount_options->flags |= CEPH_MOUNT_OPT_##opt

I think we ought to wait to merge this into mainline just yet, but I'd
like to leave it in the "testing" branch for now. 

I've been working this bug with Patrick for the last month or two:

    https://tracker.ceph.com/issues/51279

In at least one case, the problem seems to be that the MDS failed an
async create with an ENOSPC error. The kclient's error handling around
this is pretty non-existent right now, so it caused an unmount to hang. 

It's a pity we still don't have revoke()...

I've started working on a series to clean this up, but in the meantime I
think we ought to wait until that's in place before we make nowsync the
default.

Sound ok?
-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ceph: enable async dirops by default
  2021-09-01 16:54 ` Jeff Layton
@ 2021-09-02 15:52   ` Ilya Dryomov
  2021-09-07 21:07   ` Patrick Donnelly
  1 sibling, 0 replies; 10+ messages in thread
From: Ilya Dryomov @ 2021-09-02 15:52 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Ceph Development, Patrick Donnelly

On Wed, Sep 1, 2021 at 6:54 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Mon, 2021-08-09 at 12:44 -0400, Jeff Layton wrote:
> > Async dirops have been supported in mainline kernels for quite some time
> > now, and we've recently (as of June) started doing regular testing in
> > teuthology with '-o nowsync'. So far, that hasn't uncovered any issues,
> > so I think the time is right to flip the default for this option.
> >
> > Enable async dirops by default, and change /proc/mounts to show "wsync"
> > when they are disabled rather than "nowsync" when they are enabled.
> >
> > Cc: Patrick Donnelly <pdonnell@redhat.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/super.c | 4 ++--
> >  fs/ceph/super.h | 3 ++-
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > index 609ffc8c2d78..f517ad9eeb26 100644
> > --- a/fs/ceph/super.c
> > +++ b/fs/ceph/super.c
> > @@ -698,8 +698,8 @@ static int ceph_show_options(struct seq_file *m, struct dentry *root)
> >       if (fsopt->flags & CEPH_MOUNT_OPT_CLEANRECOVER)
> >               seq_show_option(m, "recover_session", "clean");
> >
> > -     if (fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS)
> > -             seq_puts(m, ",nowsync");
> > +     if (!(fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS))
> > +             seq_puts(m, ",wsync");
> >
> >       if (fsopt->wsize != CEPH_MAX_WRITE_SIZE)
> >               seq_printf(m, ",wsize=%u", fsopt->wsize);
> > diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> > index 389b45ac291b..0bc36cf4c683 100644
> > --- a/fs/ceph/super.h
> > +++ b/fs/ceph/super.h
> > @@ -48,7 +48,8 @@
> >
> >  #define CEPH_MOUNT_OPT_DEFAULT                       \
> >       (CEPH_MOUNT_OPT_DCACHE |                \
> > -      CEPH_MOUNT_OPT_NOCOPYFROM)
> > +      CEPH_MOUNT_OPT_NOCOPYFROM |            \
> > +      CEPH_MOUNT_OPT_ASYNC_DIROPS)
> >
> >  #define ceph_set_mount_opt(fsc, opt) \
> >       (fsc)->mount_options->flags |= CEPH_MOUNT_OPT_##opt
>
> I think we ought to wait to merge this into mainline just yet, but I'd
> like to leave it in the "testing" branch for now.
>
> I've been working this bug with Patrick for the last month or two:
>
>     https://tracker.ceph.com/issues/51279
>
> In at least one case, the problem seems to be that the MDS failed an
> async create with an ENOSPC error. The kclient's error handling around
> this is pretty non-existent right now, so it caused an unmount to hang.
>
> It's a pity we still don't have revoke()...
>
> I've started working on a series to clean this up, but in the meantime I
> think we ought to wait until that's in place before we make nowsync the
> default.
>
> Sound ok?

Yeah, I don't see any rush to make it the default especially given that
it hasn't been on in testing that long.

Thanks,

                Ilya

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ceph: enable async dirops by default
  2021-09-01 16:54 ` Jeff Layton
  2021-09-02 15:52   ` Ilya Dryomov
@ 2021-09-07 21:07   ` Patrick Donnelly
  1 sibling, 0 replies; 10+ messages in thread
From: Patrick Donnelly @ 2021-09-07 21:07 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Ceph Development, Ilya Dryomov

On Wed, Sep 1, 2021 at 12:54 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Mon, 2021-08-09 at 12:44 -0400, Jeff Layton wrote:
> > Async dirops have been supported in mainline kernels for quite some time
> > now, and we've recently (as of June) started doing regular testing in
> > teuthology with '-o nowsync'. So far, that hasn't uncovered any issues,
> > so I think the time is right to flip the default for this option.
> >
> > Enable async dirops by default, and change /proc/mounts to show "wsync"
> > when they are disabled rather than "nowsync" when they are enabled.
> >
> > Cc: Patrick Donnelly <pdonnell@redhat.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/super.c | 4 ++--
> >  fs/ceph/super.h | 3 ++-
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > index 609ffc8c2d78..f517ad9eeb26 100644
> > --- a/fs/ceph/super.c
> > +++ b/fs/ceph/super.c
> > @@ -698,8 +698,8 @@ static int ceph_show_options(struct seq_file *m, struct dentry *root)
> >       if (fsopt->flags & CEPH_MOUNT_OPT_CLEANRECOVER)
> >               seq_show_option(m, "recover_session", "clean");
> >
> > -     if (fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS)
> > -             seq_puts(m, ",nowsync");
> > +     if (!(fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS))
> > +             seq_puts(m, ",wsync");
> >
> >       if (fsopt->wsize != CEPH_MAX_WRITE_SIZE)
> >               seq_printf(m, ",wsize=%u", fsopt->wsize);
> > diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> > index 389b45ac291b..0bc36cf4c683 100644
> > --- a/fs/ceph/super.h
> > +++ b/fs/ceph/super.h
> > @@ -48,7 +48,8 @@
> >
> >  #define CEPH_MOUNT_OPT_DEFAULT                       \
> >       (CEPH_MOUNT_OPT_DCACHE |                \
> > -      CEPH_MOUNT_OPT_NOCOPYFROM)
> > +      CEPH_MOUNT_OPT_NOCOPYFROM |            \
> > +      CEPH_MOUNT_OPT_ASYNC_DIROPS)
> >
> >  #define ceph_set_mount_opt(fsc, opt) \
> >       (fsc)->mount_options->flags |= CEPH_MOUNT_OPT_##opt
>
> I think we ought to wait to merge this into mainline just yet, but I'd
> like to leave it in the "testing" branch for now.
>
> I've been working this bug with Patrick for the last month or two:
>
>     https://tracker.ceph.com/issues/51279
>
> In at least one case, the problem seems to be that the MDS failed an
> async create with an ENOSPC error. The kclient's error handling around
> this is pretty non-existent right now, so it caused an unmount to hang.
>
> It's a pity we still don't have revoke()...
>
> I've started working on a series to clean this up, but in the meantime I
> think we ought to wait until that's in place before we make nowsync the
> default.
>
> Sound ok?

Sounds good to me.


-- 
Patrick Donnelly, Ph.D.
He / Him / His
Principal Software Engineer
Red Hat Sunnyvale, CA
GPG: 19F28A586F808C2402351B93C3301A3E258DD79D


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ceph: enable async dirops by default
  2021-01-20 12:24     ` Ilya Dryomov
@ 2021-02-17  2:41       ` Patrick Donnelly
  0 siblings, 0 replies; 10+ messages in thread
From: Patrick Donnelly @ 2021-02-17  2:41 UTC (permalink / raw)
  To: Ilya Dryomov; +Cc: Jeff Layton, Ceph Development

On Wed, Jan 20, 2021 at 4:24 AM Ilya Dryomov <idryomov@gmail.com> wrote:
>
> On Wed, Jan 20, 2021 at 1:00 PM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > On Wed, 2021-01-20 at 11:46 +0100, Ilya Dryomov wrote:
> > > On Tue, Jan 19, 2021 at 4:06 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > >
> > > > This has been behaving reasonably well in testing, and enabling this
> > > > offers significant performance benefits. Enable async dirops by default
> > > > in the kclient going forward, and change show_options to add "wsync"
> > > > when they are disabled.
> > > >
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >  fs/ceph/super.c | 4 ++--
> > > >  fs/ceph/super.h | 5 +++--
> > > >  2 files changed, 5 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > > > index 9b1b7f4cfdd4..884e2ffabfaf 100644
> > > > --- a/fs/ceph/super.c
> > > > +++ b/fs/ceph/super.c
> > > > @@ -577,8 +577,8 @@ static int ceph_show_options(struct seq_file *m, struct dentry *root)
> > > >         if (fsopt->flags & CEPH_MOUNT_OPT_CLEANRECOVER)
> > > >                 seq_show_option(m, "recover_session", "clean");
> > > >
> > > > -       if (fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS)
> > > > -               seq_puts(m, ",nowsync");
> > > > +       if (!(fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS))
> > > > +               seq_puts(m, ",wsync");
> > > >
> > > >         if (fsopt->wsize != CEPH_MAX_WRITE_SIZE)
> > > >                 seq_printf(m, ",wsize=%u", fsopt->wsize);
> > > > diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> > > > index 13b02887b085..8ee2745f6257 100644
> > > > --- a/fs/ceph/super.h
> > > > +++ b/fs/ceph/super.h
> > > > @@ -46,8 +46,9 @@
> > > >  #define CEPH_MOUNT_OPT_ASYNC_DIROPS    (1<<15) /* allow async directory ops */
> > > >
> > > >  #define CEPH_MOUNT_OPT_DEFAULT                 \
> > > > -       (CEPH_MOUNT_OPT_DCACHE |                \
> > > > -        CEPH_MOUNT_OPT_NOCOPYFROM)
> > > > +       (CEPH_MOUNT_OPT_DCACHE          |       \
> > > > +        CEPH_MOUNT_OPT_NOCOPYFROM      |       \
> > > > +        CEPH_MOUNT_OPT_ASYNC_DIROPS)
> > > >
> > > >  #define ceph_set_mount_opt(fsc, opt) \
> > > >         (fsc)->mount_options->flags |= CEPH_MOUNT_OPT_##opt
> > > > --
> > > > 2.29.2
> > > >
> > >
> > > Hi Jeff,
> > >
> > > Is it being tested by teuthology?   I see commit 4181742a3ba8 ("qa:
> > > allow arbitrary mount options on kclient mounts"), but nothing beyond
> > > that.  I think "nowsync" needs to be turned on and get at least some
> > > nightly coverage before the default is flipped.
> > >
> > > Thanks,
> > >
> > >                 Ilya
> >
> > Good point. I had thought Patrick had added a qa variant that turned
> > that on, but I don't think that ever got merged. We definitely need that
> > enabled in QA before we make this the default.
> >
> > The catch is that we probably don't _always_ want nowsync enabled, so is
> > there some way to randomize this? Or do we need some sort of yaml file
> > that turns this on by request? What should we be aiming to do for this?
>
> It can be randomized, you can choose to run a particular set of
> jobs with both wsync and nowsync and the rest only with wsync or the
> other way around, etc.  Completely up to you, depending on the sort
> of coverage you want to get (weighted by the number of jobs added to
> the suite).

I wrote up a quick PR for this: https://github.com/ceph/ceph/pull/39505

-- 
Patrick Donnelly, Ph.D.
He / Him / His
Principal Software Engineer
Red Hat Sunnyvale, CA
GPG: 19F28A586F808C2402351B93C3301A3E258DD79D


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ceph: enable async dirops by default
  2021-01-20 12:00   ` Jeff Layton
@ 2021-01-20 12:24     ` Ilya Dryomov
  2021-02-17  2:41       ` Patrick Donnelly
  0 siblings, 1 reply; 10+ messages in thread
From: Ilya Dryomov @ 2021-01-20 12:24 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Ceph Development, Patrick Donnelly

On Wed, Jan 20, 2021 at 1:00 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Wed, 2021-01-20 at 11:46 +0100, Ilya Dryomov wrote:
> > On Tue, Jan 19, 2021 at 4:06 PM Jeff Layton <jlayton@kernel.org> wrote:
> > >
> > > This has been behaving reasonably well in testing, and enabling this
> > > offers significant performance benefits. Enable async dirops by default
> > > in the kclient going forward, and change show_options to add "wsync"
> > > when they are disabled.
> > >
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/ceph/super.c | 4 ++--
> > >  fs/ceph/super.h | 5 +++--
> > >  2 files changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > > index 9b1b7f4cfdd4..884e2ffabfaf 100644
> > > --- a/fs/ceph/super.c
> > > +++ b/fs/ceph/super.c
> > > @@ -577,8 +577,8 @@ static int ceph_show_options(struct seq_file *m, struct dentry *root)
> > >         if (fsopt->flags & CEPH_MOUNT_OPT_CLEANRECOVER)
> > >                 seq_show_option(m, "recover_session", "clean");
> > >
> > > -       if (fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS)
> > > -               seq_puts(m, ",nowsync");
> > > +       if (!(fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS))
> > > +               seq_puts(m, ",wsync");
> > >
> > >         if (fsopt->wsize != CEPH_MAX_WRITE_SIZE)
> > >                 seq_printf(m, ",wsize=%u", fsopt->wsize);
> > > diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> > > index 13b02887b085..8ee2745f6257 100644
> > > --- a/fs/ceph/super.h
> > > +++ b/fs/ceph/super.h
> > > @@ -46,8 +46,9 @@
> > >  #define CEPH_MOUNT_OPT_ASYNC_DIROPS    (1<<15) /* allow async directory ops */
> > >
> > >  #define CEPH_MOUNT_OPT_DEFAULT                 \
> > > -       (CEPH_MOUNT_OPT_DCACHE |                \
> > > -        CEPH_MOUNT_OPT_NOCOPYFROM)
> > > +       (CEPH_MOUNT_OPT_DCACHE          |       \
> > > +        CEPH_MOUNT_OPT_NOCOPYFROM      |       \
> > > +        CEPH_MOUNT_OPT_ASYNC_DIROPS)
> > >
> > >  #define ceph_set_mount_opt(fsc, opt) \
> > >         (fsc)->mount_options->flags |= CEPH_MOUNT_OPT_##opt
> > > --
> > > 2.29.2
> > >
> >
> > Hi Jeff,
> >
> > Is it being tested by teuthology?   I see commit 4181742a3ba8 ("qa:
> > allow arbitrary mount options on kclient mounts"), but nothing beyond
> > that.  I think "nowsync" needs to be turned on and get at least some
> > nightly coverage before the default is flipped.
> >
> > Thanks,
> >
> >                 Ilya
>
> Good point. I had thought Patrick had added a qa variant that turned
> that on, but I don't think that ever got merged. We definitely need that
> enabled in QA before we make this the default.
>
> The catch is that we probably don't _always_ want nowsync enabled, so is
> there some way to randomize this? Or do we need some sort of yaml file
> that turns this on by request? What should we be aiming to do for this?

It can be randomized, you can choose to run a particular set of
jobs with both wsync and nowsync and the rest only with wsync or the
other way around, etc.  Completely up to you, depending on the sort
of coverage you want to get (weighted by the number of jobs added to
the suite).

Thanks,

                Ilya

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ceph: enable async dirops by default
  2021-01-20 10:46 ` Ilya Dryomov
@ 2021-01-20 12:00   ` Jeff Layton
  2021-01-20 12:24     ` Ilya Dryomov
  0 siblings, 1 reply; 10+ messages in thread
From: Jeff Layton @ 2021-01-20 12:00 UTC (permalink / raw)
  To: Ilya Dryomov; +Cc: Ceph Development, Patrick Donnelly

On Wed, 2021-01-20 at 11:46 +0100, Ilya Dryomov wrote:
> On Tue, Jan 19, 2021 at 4:06 PM Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > This has been behaving reasonably well in testing, and enabling this
> > offers significant performance benefits. Enable async dirops by default
> > in the kclient going forward, and change show_options to add "wsync"
> > when they are disabled.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/super.c | 4 ++--
> >  fs/ceph/super.h | 5 +++--
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > index 9b1b7f4cfdd4..884e2ffabfaf 100644
> > --- a/fs/ceph/super.c
> > +++ b/fs/ceph/super.c
> > @@ -577,8 +577,8 @@ static int ceph_show_options(struct seq_file *m, struct dentry *root)
> >         if (fsopt->flags & CEPH_MOUNT_OPT_CLEANRECOVER)
> >                 seq_show_option(m, "recover_session", "clean");
> > 
> > -       if (fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS)
> > -               seq_puts(m, ",nowsync");
> > +       if (!(fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS))
> > +               seq_puts(m, ",wsync");
> > 
> >         if (fsopt->wsize != CEPH_MAX_WRITE_SIZE)
> >                 seq_printf(m, ",wsize=%u", fsopt->wsize);
> > diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> > index 13b02887b085..8ee2745f6257 100644
> > --- a/fs/ceph/super.h
> > +++ b/fs/ceph/super.h
> > @@ -46,8 +46,9 @@
> >  #define CEPH_MOUNT_OPT_ASYNC_DIROPS    (1<<15) /* allow async directory ops */
> > 
> >  #define CEPH_MOUNT_OPT_DEFAULT                 \
> > -       (CEPH_MOUNT_OPT_DCACHE |                \
> > -        CEPH_MOUNT_OPT_NOCOPYFROM)
> > +       (CEPH_MOUNT_OPT_DCACHE          |       \
> > +        CEPH_MOUNT_OPT_NOCOPYFROM      |       \
> > +        CEPH_MOUNT_OPT_ASYNC_DIROPS)
> > 
> >  #define ceph_set_mount_opt(fsc, opt) \
> >         (fsc)->mount_options->flags |= CEPH_MOUNT_OPT_##opt
> > --
> > 2.29.2
> > 
> 
> Hi Jeff,
> 
> Is it being tested by teuthology?   I see commit 4181742a3ba8 ("qa:
> allow arbitrary mount options on kclient mounts"), but nothing beyond
> that.  I think "nowsync" needs to be turned on and get at least some
> nightly coverage before the default is flipped.
> 
> Thanks,
> 
>                 Ilya

Good point. I had thought Patrick had added a qa variant that turned
that on, but I don't think that ever got merged. We definitely need that
enabled in QA before we make this the default.

The catch is that we probably don't _always_ want nowsync enabled, so is
there some way to randomize this? Or do we need some sort of yaml file
that turns this on by request? What should we be aiming to do for this?

How are the existing, more obscure mount options usually tested in
teuthology today?
-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ceph: enable async dirops by default
  2021-01-19 14:44 Jeff Layton
  2021-01-20 10:46 ` Ilya Dryomov
@ 2021-01-20 11:07 ` Luis Henriques
  1 sibling, 0 replies; 10+ messages in thread
From: Luis Henriques @ 2021-01-20 11:07 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel

Jeff Layton <jlayton@kernel.org> writes:

> This has been behaving reasonably well in testing, and enabling this
> offers significant performance benefits. Enable async dirops by default
> in the kclient going forward, and change show_options to add "wsync"
> when they are disabled.

Sounds good!  Maybe it's worth adding this option to the mount options
listed in Documentation/filesystems/ceph.rst (although it's quite possible
that other mount options are already missing there).

Cheers,
-- 
Luis

>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/super.c | 4 ++--
>  fs/ceph/super.h | 5 +++--
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 9b1b7f4cfdd4..884e2ffabfaf 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -577,8 +577,8 @@ static int ceph_show_options(struct seq_file *m, struct dentry *root)
>  	if (fsopt->flags & CEPH_MOUNT_OPT_CLEANRECOVER)
>  		seq_show_option(m, "recover_session", "clean");
>  
> -	if (fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS)
> -		seq_puts(m, ",nowsync");
> +	if (!(fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS))
> +		seq_puts(m, ",wsync");
>  
>  	if (fsopt->wsize != CEPH_MAX_WRITE_SIZE)
>  		seq_printf(m, ",wsize=%u", fsopt->wsize);
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index 13b02887b085..8ee2745f6257 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -46,8 +46,9 @@
>  #define CEPH_MOUNT_OPT_ASYNC_DIROPS    (1<<15) /* allow async directory ops */
>  
>  #define CEPH_MOUNT_OPT_DEFAULT			\
> -	(CEPH_MOUNT_OPT_DCACHE |		\
> -	 CEPH_MOUNT_OPT_NOCOPYFROM)
> +	(CEPH_MOUNT_OPT_DCACHE		|	\
> +	 CEPH_MOUNT_OPT_NOCOPYFROM	|	\
> +	 CEPH_MOUNT_OPT_ASYNC_DIROPS)
>  
>  #define ceph_set_mount_opt(fsc, opt) \
>  	(fsc)->mount_options->flags |= CEPH_MOUNT_OPT_##opt
> -- 
>
> 2.29.2
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ceph: enable async dirops by default
  2021-01-19 14:44 Jeff Layton
@ 2021-01-20 10:46 ` Ilya Dryomov
  2021-01-20 12:00   ` Jeff Layton
  2021-01-20 11:07 ` Luis Henriques
  1 sibling, 1 reply; 10+ messages in thread
From: Ilya Dryomov @ 2021-01-20 10:46 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Ceph Development

On Tue, Jan 19, 2021 at 4:06 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> This has been behaving reasonably well in testing, and enabling this
> offers significant performance benefits. Enable async dirops by default
> in the kclient going forward, and change show_options to add "wsync"
> when they are disabled.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/super.c | 4 ++--
>  fs/ceph/super.h | 5 +++--
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 9b1b7f4cfdd4..884e2ffabfaf 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -577,8 +577,8 @@ static int ceph_show_options(struct seq_file *m, struct dentry *root)
>         if (fsopt->flags & CEPH_MOUNT_OPT_CLEANRECOVER)
>                 seq_show_option(m, "recover_session", "clean");
>
> -       if (fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS)
> -               seq_puts(m, ",nowsync");
> +       if (!(fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS))
> +               seq_puts(m, ",wsync");
>
>         if (fsopt->wsize != CEPH_MAX_WRITE_SIZE)
>                 seq_printf(m, ",wsize=%u", fsopt->wsize);
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index 13b02887b085..8ee2745f6257 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -46,8 +46,9 @@
>  #define CEPH_MOUNT_OPT_ASYNC_DIROPS    (1<<15) /* allow async directory ops */
>
>  #define CEPH_MOUNT_OPT_DEFAULT                 \
> -       (CEPH_MOUNT_OPT_DCACHE |                \
> -        CEPH_MOUNT_OPT_NOCOPYFROM)
> +       (CEPH_MOUNT_OPT_DCACHE          |       \
> +        CEPH_MOUNT_OPT_NOCOPYFROM      |       \
> +        CEPH_MOUNT_OPT_ASYNC_DIROPS)
>
>  #define ceph_set_mount_opt(fsc, opt) \
>         (fsc)->mount_options->flags |= CEPH_MOUNT_OPT_##opt
> --
> 2.29.2
>

Hi Jeff,

Is it being tested by teuthology?   I see commit 4181742a3ba8 ("qa:
allow arbitrary mount options on kclient mounts"), but nothing beyond
that.  I think "nowsync" needs to be turned on and get at least some
nightly coverage before the default is flipped.

Thanks,

                Ilya

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] ceph: enable async dirops by default
@ 2021-01-19 14:44 Jeff Layton
  2021-01-20 10:46 ` Ilya Dryomov
  2021-01-20 11:07 ` Luis Henriques
  0 siblings, 2 replies; 10+ messages in thread
From: Jeff Layton @ 2021-01-19 14:44 UTC (permalink / raw)
  To: ceph-devel

This has been behaving reasonably well in testing, and enabling this
offers significant performance benefits. Enable async dirops by default
in the kclient going forward, and change show_options to add "wsync"
when they are disabled.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/super.c | 4 ++--
 fs/ceph/super.h | 5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 9b1b7f4cfdd4..884e2ffabfaf 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -577,8 +577,8 @@ static int ceph_show_options(struct seq_file *m, struct dentry *root)
 	if (fsopt->flags & CEPH_MOUNT_OPT_CLEANRECOVER)
 		seq_show_option(m, "recover_session", "clean");
 
-	if (fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS)
-		seq_puts(m, ",nowsync");
+	if (!(fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS))
+		seq_puts(m, ",wsync");
 
 	if (fsopt->wsize != CEPH_MAX_WRITE_SIZE)
 		seq_printf(m, ",wsize=%u", fsopt->wsize);
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index 13b02887b085..8ee2745f6257 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -46,8 +46,9 @@
 #define CEPH_MOUNT_OPT_ASYNC_DIROPS    (1<<15) /* allow async directory ops */
 
 #define CEPH_MOUNT_OPT_DEFAULT			\
-	(CEPH_MOUNT_OPT_DCACHE |		\
-	 CEPH_MOUNT_OPT_NOCOPYFROM)
+	(CEPH_MOUNT_OPT_DCACHE		|	\
+	 CEPH_MOUNT_OPT_NOCOPYFROM	|	\
+	 CEPH_MOUNT_OPT_ASYNC_DIROPS)
 
 #define ceph_set_mount_opt(fsc, opt) \
 	(fsc)->mount_options->flags |= CEPH_MOUNT_OPT_##opt
-- 
2.29.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-09-07 21:07 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-09 16:44 [PATCH] ceph: enable async dirops by default Jeff Layton
2021-09-01 16:54 ` Jeff Layton
2021-09-02 15:52   ` Ilya Dryomov
2021-09-07 21:07   ` Patrick Donnelly
  -- strict thread matches above, loose matches on Subject: below --
2021-01-19 14:44 Jeff Layton
2021-01-20 10:46 ` Ilya Dryomov
2021-01-20 12:00   ` Jeff Layton
2021-01-20 12:24     ` Ilya Dryomov
2021-02-17  2:41       ` Patrick Donnelly
2021-01-20 11:07 ` Luis Henriques

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).