Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 29 Jun 2022 14:37:56 -0400
From:   Vivek Goyal <vgoyal@redhat.com>
To:     Amir Goldstein <amir73il@gmail.com>
Cc:     Jiachen Zhang <zhangjiachen.jaycee@bytedance.com>,
        Miklos Szeredi <miklos@szeredi.hu>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        xieyongji@bytedance.com, fam.zheng@bytedance.com,
        Miklos Szeredi <mszeredi@redhat.com>
Subject: Re: [PATCH] fuse: writeback_cache consistency enhancement
 (writeback_cache_v2)
Message-ID: <YrycBC8Kn3S/kxQP@redhat.com>
References: <20220624055825.29183-1-zhangjiachen.jaycee@bytedance.com>
 <CAOQ4uxjbU27gSbhndxA_ABchfv1X7Steoggto_o-Wc=5shfS7Q@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAOQ4uxjbU27gSbhndxA_ABchfv1X7Steoggto_o-Wc=5shfS7Q@mail.gmail.com>
Precedence: bulk

On Tue, Jun 28, 2022 at 01:09:56PM +0300, Amir Goldstein wrote:
> On Fri, Jun 24, 2022 at 9:03 AM Jiachen Zhang
> <zhangjiachen.jaycee@bytedance.com> wrote:
> >
> > Some users may want both the high performance of the writeback_cahe mode and
> > a little bit more consistency among FUSE mounts. In the current writeback
> > mode implementation, users of one FUSE mount can never see the file
> > expansion done by other FUSE mounts.
> >
> > Based on the suggested writeback V2 patch in the upstream mailing-list [1],
> > this commit allows the cmtime and size to be updated from server in
> > writeback mode. Compared with the writeback V2 patch in [1], this commit has
> > several differences:
> >
> >     1. Ensure c/mtime are not updated from kernel to server. IOW, the cmtime
> >     generated by kernel are just temporary values that are never flushed to
> >     server, and they can also be updated by the official server cmtime when
> >     the writeback cache is clean.
> >
> >     2. Skip mtime-based revalidation when fc->auto_inval_data is set with
> >     fc->writeback_cache_v2. Because the kernel-generated temporary cmtime
> >     are likely not equal to the offical server cmtime.
> >
> >     3. If any page is ever flushed to the server during FUSE_GETATTR
> >     handling on fuse server, even if the cache is clean when
> >     fuse_change_attributes() checks, we should not update the i_size. This
> >     is because the FUSE_GETATTR may get a staled size before the FUSE_WRITE
> >     request changes server inode size. This commit ensures this by
> >     increasing attr_version after writeback for writeback_cache_v2. In that
> >     case, we should also ensure the ordering of the attr_version updating
> >     and the fi->writepages RB-tree updating. So that if a fuse page
> >     writeback ever happens during fuse_change_attributes(), either the
> >     fi->writepages is not empty, or the attr_version is increased. So we
> >     never mistakenly update a stale file size from server to kernel.
> >
> > With this patch, writeback mode can consider the server c/mtime as the
> > official one. When inode attr is timeout or invalidated, kernel has chance
> > to see size and c/mtime modified by others.
> >
> > Together with another patch [2], a FUSE daemon is able to implement
> > close-to-open (CTO) consistency like what is done in NFS clients.
> >
> > [1] https://lore.kernel.org/linux-fsdevel/Ymfu8fGbfYi4FxQ4@miu.piliscsaba.redhat.com
> > [2] https://lore.kernel.org/linux-fsdevel/20220608104202.19461-1-zhangjiachen.jaycee@bytedance.com/
> >
> > Suggested-by: Miklos Szeredi <mszeredi@redhat.com>
> > Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com>
> > ---
> >  fs/fuse/file.c            | 17 +++++++++++++++
> >  fs/fuse/fuse_i.h          |  3 +++
> >  fs/fuse/inode.c           | 44 +++++++++++++++++++++++++++++++++++++--
> >  include/uapi/linux/fuse.h |  5 +++++
> >  4 files changed, 67 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index 9b64e2ff1c96..35bdc7af8468 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -1829,6 +1829,15 @@ static void fuse_writepage_end(struct fuse_mount *fm, struct fuse_args *args,
> >                  */
> >                 fuse_send_writepage(fm, next, inarg->offset + inarg->size);
> >         }
> > +
> > +       if (fc->writeback_cache_v2)
> > +               fi->attr_version = atomic64_inc_return(&fc->attr_version);
> > +       /*
> > +        * Ensure attr_version increases before the page is move out of the
> > +        * writepages rb-tree.
> > +        */
> > +       smp_mb();
> > +
> >         fi->writectr--;
> >         fuse_writepage_finish(fm, wpa);
> >         spin_unlock(&fi->lock);
> > @@ -1858,10 +1867,18 @@ static struct fuse_file *fuse_write_file_get(struct fuse_inode *fi)
> >
> >  int fuse_write_inode(struct inode *inode, struct writeback_control *wbc)
> >  {
> > +       struct fuse_conn *fc = get_fuse_conn(inode);
> >         struct fuse_inode *fi = get_fuse_inode(inode);
> >         struct fuse_file *ff;
> >         int err;
> >
> > +       /*
> > +        * Kernel c/mtime should not be updated to the server in the
> > +        * writeback_cache_v2 mode as server c/mtime are official.
> > +        */
> > +       if (fc->writeback_cache_v2)
> > +               return 0;
> > +
> >         /*
> >          * Inode is always written before the last reference is dropped and
> >          * hence this should not be reached from reclaim.
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 488b460e046f..47de36146fb8 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -654,6 +654,9 @@ struct fuse_conn {
> >         /* show legacy mount options */
> >         unsigned int legacy_opts_show:1;
> >
> > +       /* Improved writeback cache policy */
> > +       unsigned writeback_cache_v2:1;
> > +
> 
> Seeing that writeback_cache_v2 depends on writeback_cache
> I wonder whether that will not be better represented as:
> 
>         /** write-back cache policy (default is write-through) */
> -       unsigned writeback_cache:1;
> +      unsigned writeback_cache:2;
> 
> 
> Looking at the recently added handle_killpriv_v2, I also wonder
> if that would not have been better.
> 
> Vivek,
> is handle_killpriv_v2 really independent of handle_killpriv?
> Seeing test like these worry me as they are inviting bugs:
> 
>                 if (!fc->handle_killpriv && !fc->handle_killpriv_v2) {

Hi Amir,

IIRC, yes, handle_killpriv_v2 and handle_killpriv are independent.

Above check is doing a GETATTR request to server to get latest mode,
only if none of handle_killpriv and and handle_killpriv_v2 are enabled.
If any one of these is enabled, we skip fuse_do_getattr() and reset
ATTR_MODE and expect server to clear suid/sgid bits.

handle_killpriv was not enough, so I added handle_killpriv_v2 and
even enabled it by default in virtiofsd. Later there came reports that
some of the xfstests failed where suid/sgid was not cleared on server.
I had tried to debug it and it was some issue in underlying ext4/xfs
filesystem. I could not pursue it further at the time. I should probably
get back to it. My temporary solution was to disable killpriv_v2 in
virtiofsd.

Havind said that, I feel there are too many corner cases w.r.t suid/sgid
clearing. And it probably is a good idea that client sends a setattr
request with ATTR_MODE set. While it is racy but it can be more correct.
Otherwise it is easy to miss some code path somewhere where assumptions
are not correct and we did not clear suid/sgid as expected.

Thanks
Vivek