Re: [PATCH 2/2] Use fiemap internal infra-structure to handle FIBMAP

[Carlos Maiolino <cmaiolino@redhat.com>]

On Wed, Sep 05, 2018 at 09:40:06AM -0500, Eric Sandeen wrote:
> On 9/5/18 8:57 AM, Carlos Maiolino wrote:
> > This patch modifies ioctl_fibmap to use FIEMAP interface internally. The
> > FIBMAP API is not changed, but now ->bmap can go.
> 
> Not really, because you still call it if ->fiemap isn't there ;)

Not sure if I understand what you meant here :(

What I meant to say is that, FIBMAP behavior from user's perspective isn't
supposed to change, neither the user API which needs to be used (we should
support FIBMAP forever, and breaking user's API is a big NO here). Not sure if I
didn't understand what you meant here, or maybe we are talking about different
things? =/

> 
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> >  fs/inode.c                  | 38 +++++++++++++++++++++++++++++++++++++-
> >  fs/ioctl.c                  | 11 +++++++++--
> >  include/uapi/linux/fiemap.h |  1 +
> >  3 files changed, 47 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/inode.c b/fs/inode.c
> > index 41812a3e829e..07befc5f253b 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -1588,9 +1588,45 @@ EXPORT_SYMBOL(iput);
> >   */
> >  sector_t bmap(struct inode *inode, sector_t block)
> >  {
> > +
> > +	struct fiemap_extent_info fieinfo = { 0, };
> > +	struct fiemap_extent fextent;
> > +	u64 start = block << inode->i_blkbits;
> > +	int error;
> >  	sector_t res = 0;
> > -	if (inode->i_mapping->a_ops->bmap)
> > +
> > +	if (inode->i_op->fiemap) {
> > +		fextent.fe_logical = 0;
> > +		fextent.fe_physical = 0;
> > +		fieinfo.fi_flags = FIEMAP_KERNEL_FIBMAP;
> 
> | FIEMAP_FLAG_SYNC?
> 
> > +		fieinfo.fi_extents_max = 1;
> > +		fieinfo.fi_extents_start = (__force struct fiemap_extent __user *) &fextent;
> > +
> > +		error = inode->i_op->fiemap(inode, &fieinfo, start, 1);
> > +
> > +		/* FIBMAP expect a zero return for error */
> > +		if (error)
> > +			goto out;
> 
> do you need to test fieinfo.fi_extents_mapped > 0?  I don't know if it's
> possible to return error == 0 && fi_extents_mapped == 0, TBH.
> 
> > +
> > +		/*
> > +		 * Fiemap implementation of some filesystems will return the
> > +		 * extent map beginning at the requested offset
> > +		 * (fextent.fi_logical == start), while other FSes will return
> > +		 * the beginning of the extent at fextent.fe_logical, even if
> > +		 * the requested offset is somehwere in the middle of the
> > +		 * extent. We must be sure to return the correct block block
> > +		 * here in both cases.
> > +		 */
> > +		if (fextent.fe_logical != start)
> > +			res = (fextent.fe_physical + start) >> inode->i_blkbits;
> 
> I don't think this is correct, is it?  You can't add the entire requested
> logical file offset (start) to the resulting physical extent start (fe_physical).
> 
> You'd need to add the delta, (start - fextent.fe_logical) to the physical extent
> start to get the offset into the mapping, right?
> 
> 		res = (fextent.fe_physical +
> 		       (start - fextent.fe_logical)) >> inode->i_blkbits;

I believe you're right, yes, when I wrote the calculation above I was thinking
about "fe_logical is either 0 or equal start", and I tested it against
multi-extents files. I really have no idea how it survived my tests and xfstests
=/

> 
> You'll also need to test the resulting fm_flags to be sure that
> this is something that's valid to return via FIBMAP.
> 
> For example, you might get a shared block, which we have recently learned
> Must Not Be Provided Under Any Circumstances to userspace via FIBMAP.

Right, good point.

> 
> (I'm still thinking about how this change is structured in general, but I think
> the above are a few logical issues w/ the code.)
> 

Yes, makes sense, and I'm still also thinking on a way to better structure it.
And that's the point I decided to send an RFC, better to start a conversation
with a somehow working patch. Thanks for the input, much appreciated.

> -Eric
> 
> > +		else
> > +			res = fextent.fe_physical >> inode->i_blkbits;
> > +
> > +	} else if (inode->i_mapping->a_ops->bmap) {
> >  		res = inode->i_mapping->a_ops->bmap(inode->i_mapping, block);
> > +	}
> > +
> > +out:
> >  	return res;
> >  }
> >  EXPORT_SYMBOL(bmap);
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index 413585d58415..58de1dd507b1 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -117,8 +117,14 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> >  	extent.fe_flags = flags;
> >  
> >  	dest += fieinfo->fi_extents_mapped;
> > -	if (copy_to_user(dest, &extent, sizeof(extent)))
> > -		return -EFAULT;
> > +
> > +	if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP) {
> > +		memcpy((__force struct fiemap_extent *)dest, &extent,
> > +		       sizeof(extent));
> > +	} else {
> > +		if (copy_to_user(dest, &extent, sizeof(extent)))
> > +			return -EFAULT;
> > +	}
> >  
> >  	fieinfo->fi_extents_mapped++;
> >  	if (fieinfo->fi_extents_mapped == fieinfo->fi_extents_max)
> > @@ -146,6 +152,7 @@ int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags)
> >  	u32 incompat_flags;
> >  
> >  	incompat_flags = fieinfo->fi_flags & ~(FIEMAP_FLAGS_COMPAT & fs_flags);
> > +	incompat_flags &= ~(FIEMAP_KERNEL_FIBMAP);
> >  	if (incompat_flags) {
> >  		fieinfo->fi_flags = incompat_flags;
> >  		return -EBADR;
> > diff --git a/include/uapi/linux/fiemap.h b/include/uapi/linux/fiemap.h
> > index 8c0bc24d5d95..7064a3fa5421 100644
> > --- a/include/uapi/linux/fiemap.h
> > +++ b/include/uapi/linux/fiemap.h
> > @@ -42,6 +42,7 @@ struct fiemap {
> >  #define FIEMAP_FLAG_SYNC	0x00000001 /* sync file data before map */
> >  #define FIEMAP_FLAG_XATTR	0x00000002 /* map extended attribute tree */
> >  #define FIEMAP_FLAG_CACHE	0x00000004 /* request caching of the extents */
> > +#define FIEMAP_KERNEL_FIBMAP	0x00000008 /* Use FIEMAP for FIBMAP */
> >  
> >  #define FIEMAP_FLAGS_COMPAT	(FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR)
> >  
> > 
> 

-- 
Carlos