Re: [Virtio-fs] [PATCH v3 0/2] Fix xattr operation

[Vivek Goyal <vgoyal@redhat.com>]

On Thu, Feb 27, 2020 at 05:16:21AM +0000, misono.tomohiro@fujitsu.com wrote:
> > On Thu, Feb 20, 2020 at 08:47:02PM +0900, Misono Tomohiro wrote:
> > > This fixes the xattr operation for directory and special files (which
> > > can be tested by xfstests generic/062 with -o xattr option).
> > >
> > > The overall logic is switched back to the same as v1 in favor of
> > > performance (i.e. keep original implementation for regular
> > > files/directories) but I add a cleanup patch to improve readability as requested by Vivek.
> > >
> > > Known issue is that if xattr enabled, seek sanity tests (generic/285,
> > > 436) will fail. However, I understand this is not a very serious bug
> > > like data corruption so leave it for now.
> > 
> > Hi Misono,
> > 
> > Do you know why generic/285 and generic/436 fail with xattr enabled.  Is it something easily fixable later.
> 
> Not sure if it can be fixed easily, but here is what I figured out:
> 
> The failure case checks lseek SEEK_DATA/SEEK_HOLE with some unwritten extent (by fallocate).
> I notice that open/close affects lseek behavior in that case on xfs: 
> 
> # on xfs
>  xfs_io> open -ft temp
>  xfs_io> falloc 0 40k
>  xfs_io> seek -a 0 # show both SEEK_HOLE/SEEK_DATA with offset 0
>  Whence  Result
>  HOLE    0      # hole only (fallocated block is treated as hole at this time)
> 
> # close and open again
>  xfs_io> close
>  xfs_io> open temp
>  xfs_io> seek -a 0
>  Whence  Result
>  DATA    0       # fallocated block is treated as data now
>  HOLE    40960  # hole position is now EOF
> 
> I have not yet understand what really causes this in detail (didn't check other fs neither).

Hi Misono,

This is indeed very interesting and confusing. Just opening and closing
file resulted in allocation of blocks.

Will be good if you can narrow it down a bit more and see what's causing
this.

Thanks
Vivek

> 
> Since getxattr operation happens during the test to check "security.capability"
> file attribute when writing some data, it seems this behavior affects the test on virtiofs with xfs.
> 
> Thanks.
> Misono
> 
> > 
> > >
> > > One question; I remove error handling of fchdir() in v3 since I
> > > believe fchdir to proc_self_fd/root.fd cannot fail in the situation
> > > but should I add error handling?
> > 
> > I would think its good to have error handling for fchdir() case. We don't expect it, but if something goes wrong, we will like
> > to catch it.
> > 
> > Thanks
> > Vivek
> > 
> > >
> > > change v2 -> v3:
> > >  - rebased to current dev branch
> > >  - add cleanup path (first one) to simplify main patch (second patch)
> > >  - restore the logic of v1 in favor of performance
> > >    (as a result seek sanity test failure is not fixed by this series)
> > >  - remove error handling of fchdir
> > >  - drop ACL fix included in v2 for now to focus xattr
> > >
> > > v2 patch:
> > > https://www.redhat.com/archives/virtio-fs/2020-January/msg00131.html
> > >
> > > Thanks!
> > >
> > > Misono Tomohiro (2):
> > >   virtiofs: passthrough_ll: cleanup getxattr/listxattr
> > >   virtiofs: Fix xattr operations
> > >
> > >  tools/virtiofsd/fuse_virtio.c    |  13 +++
> > >  tools/virtiofsd/passthrough_ll.c | 141 +++++++++++++++----------------
> > >  tools/virtiofsd/seccomp.c        |   6 ++
> > >  3 files changed, 87 insertions(+), 73 deletions(-)
> > >
> > > --
> > > 2.21.1
> > >
>