[Devel] fuse: call proper fiemap_prep as it is required by new kernel api

[Kui Liu]

Then ok for me. 

-----Original Message-----
From: Alexey Kuznetsov <kuznet at virtuozzo.com <mailto:kuznet at virtuozzo.com>>
Date: Monday, 16 October 2023 at 6:34 PM
To: Kui Liu <Kui.Liu at acronis.com <mailto:Kui.Liu at acronis.com>>
Cc: Devel <devel at openvz.org <mailto:devel at openvz.org>>, Andrey Zaitsev <Andrey.Zaitsev at acronis.com <mailto:Andrey.Zaitsev at acronis.com>>, "Denis V. Lunev" <den at virtuozzo.com <mailto:den at virtuozzo.com>>, Konstantin Khorenko <khorenko at virtuozzo.com <mailto:khorenko at virtuozzo.com>>
Subject: Re: fuse: call proper fiemap_prep as it is required by new kernel api


Hello!


> 1. It’s better to move fiemap_prep() before inode_lock, as it was the case before when same thing is done in ioctl_fiemap().


It is intentional. It was like a bug in older kernels that we
flushed dirty pages outside of locks so that we could came to fiemap
with new dirty pages. Not that it is of any importance, as user
calling write() and fiemap() in parallel deemed
to get bogus results.


BTW generally flush should be called twice. The first time - out of
mutex, to make bulk work out of mutex,
and then under mutex to collect bits dirtied while the fist pass. But
in this case as we flush only area
which is subject of fiemap, the first pass is mostly useless.


> FIEMAP_FLAG_SYNC as the last parameter is unnecessary, because it is always checked in fiemap_prep().


It is noop but IMHO not redundant as states intention and improves
readability w/o necessity to look
into actual function to figure out why this flag is not marked as "supported".








On Mon, Oct 16, 2023 at 6:17 PM Kui Liu <Kui.Liu at acronis.com <mailto:Kui.Liu at acronis.com>> wrote:
>
> Found 2 issues:
>
> It’s better to move fiemap_prep() before inode_lock, as it was the case before when same thing is done in ioctl_fiemap().
> FIEMAP_FLAG_SYNC as the last parameter is unnecessary, because it is always checked in fiemap_prep().
>
>
>
> From: Andrey Zaitsev <Andrey.Zaitsev at acronis.com <mailto:Andrey.Zaitsev at acronis.com>>
> Date: Monday, 16 October 2023 at 5:23 PM
> To: "Denis V. Lunev" <den at virtuozzo.com <mailto:den at virtuozzo.com>>, Alexey Kuznetsov <kuznet at virtuozzo.com <mailto:kuznet at virtuozzo.com>>, Devel <devel at openvz.org <mailto:devel at openvz.org>>, "khorenko at virtuozzo.com <mailto:khorenko at virtuozzo.com>" <khorenko at virtuozzo.com <mailto:khorenko at virtuozzo.com>>, Kui Liu <Kui.Liu at acronis.com <mailto:Kui.Liu at acronis.com>>
> Subject: Re: fuse: call proper fiemap_prep as it is required by new kernel api
>
>
>
> ok for me
>
> ________________________________
>
> From: Denis V. Lunev <den at virtuozzo.com <mailto:den at virtuozzo.com>>
> Sent: Sunday, October 15, 2023 10:08:40 PM
> To: Alexey Kuznetsov; Devel; khorenko at virtuozzo.com <mailto:khorenko at virtuozzo.com>; Andrey Zaitsev; Kui Liu
> Subject: Re: fuse: call proper fiemap_prep as it is required by new kernel api
>
>
>
> On 10/13/23 20:35, Alexey Kuznetsov wrote:
> > Otherwise we consider locations with dirty page cache as holes.
> >
> > Affects: #VSTOR-76073, #PSBM-151414, #PSBM-151424, #PSBM-151464
> >
> > Signed-off-by: Alexey Kuznetsov <kuznet at acronis.com <mailto:kuznet at acronis.com>>
> > ---
> > fs/fuse/file.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index 5e23bd5..c685e01 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -3711,6 +3711,10 @@ int fuse_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> >
> > inode_lock(inode);
> >
> > + err = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_SYNC);
> > + if (err)
> > + goto out;
> > +
> > fuse_sync_writes(inode);
> > fuse_read_dio_wait(fi);
> >
> Can we get acknowledgement for this patch in order to
> start merge/build ASAP?
>
> Thanks you in advance,
> Den