diff mbox series

ceph: enable async dirops by default

Message ID 20210119144430.337370-1-jlayton@kernel.org
State Superseded
Headers show
Series ceph: enable async dirops by default | expand

Commit Message

Jeff Layton Jan. 19, 2021, 2:44 p.m. UTC
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(-)

Comments

Ilya Dryomov Jan. 20, 2021, 10:46 a.m. UTC | #1
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
Luis Henriques Jan. 20, 2021, 11:07 a.m. UTC | #2
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

>
Jeff Layton Jan. 20, 2021, noon UTC | #3
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>
Ilya Dryomov Jan. 20, 2021, 12:24 p.m. UTC | #4
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
Patrick Donnelly Feb. 17, 2021, 2:41 a.m. UTC | #5
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
diff mbox series

Patch

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