Re: [PATCH] ceph: do not truncate pagecache if truncate size doesn't change

From: "Yan, Zheng" <ukernel@gmail.com>
To: Jeff Layton <jlayton@kernel.org>
Cc: Xiubo Li <xiubli@redhat.com>, Ilya Dryomov <idryomov@gmail.com>,
	Venky Shankar <vshankar@redhat.com>,
	ceph-devel <ceph-devel@vger.kernel.org>
Subject: Re: [PATCH] ceph: do not truncate pagecache if truncate size doesn't change
Date: Wed, 17 Nov 2021 10:47:49 +0800	[thread overview]
Message-ID: <CAAM7YAm6iovKdptjiZhgjm8kwrOeUyBYG3z82+Updrkrd-QMOA@mail.gmail.com> (raw)
In-Reply-To: <d37b49e0048ba3cf6763b83c82ad2fd8f8f36663.camel@kernel.org>

On Wed, Nov 17, 2021 at 4:13 AM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Tue, 2021-11-16 at 17:20 +0800, xiubli@redhat.com wrote:
> > From: Xiubo Li <xiubli@redhat.com>
> >
> > In case truncating a file to a smaller sizeA, the sizeA will be kept
> > in truncate_size. And if truncate the file to a bigger sizeB, the
> > MDS will only increase the truncate_seq, but still using the sizeA as
> > the truncate_size.
> >
>
> Do you mean "kept in ci->i_truncate_size" ? If so, is this really the
> correct fix? I'll note this in the sources:
>
>         u32 i_truncate_seq;        /* last truncate to smaller size */
>         u64 i_truncate_size;       /*  and the size we last truncated down to */
>
> Maybe the MDS ought not bump the truncate_seq unless it was truncating
> to a smaller size? If not, then that comment seems wrong at least.
>

mds does not change truncate_{seq,size} when truncating file to bigger size

>
> > So when filling the inode it will truncate the pagecache by using
> > truncate_sizeA again, which makes no sense and will trim the inocent
> > pages.
> >
>
> Is there a reproducer for this? It would be nice to put something in
> xfstests for it if so.
>
> > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > ---
> >  fs/ceph/inode.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > index 1b4ce453d397..b4f784684e64 100644
> > --- a/fs/ceph/inode.c
> > +++ b/fs/ceph/inode.c
> > @@ -738,10 +738,11 @@ int ceph_fill_file_size(struct inode *inode, int issued,
> >                        * don't hold those caps, then we need to check whether
> >                        * the file is either opened or mmaped
> >                        */
> > -                     if ((issued & (CEPH_CAP_FILE_CACHE|
> > +                     if (ci->i_truncate_size != truncate_size &&
> > +                         ((issued & (CEPH_CAP_FILE_CACHE|
> >                                      CEPH_CAP_FILE_BUFFER)) ||
> >                           mapping_mapped(inode->i_mapping) ||
> > -                         __ceph_is_file_opened(ci)) {
> > +                         __ceph_is_file_opened(ci))) {
> >                               ci->i_truncate_pending++;
> >                               queue_trunc = 1;
> >                       }
>
> --
> Jeff Layton <jlayton@kernel.org>