Re: [PATCH] ceph: enable async dirops by default

[Patrick Donnelly <pdonnell@redhat.com>]

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