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

[Carlos Maiolino <cmaiolino@redhat.com>]

Hi Christoph,

On Wed, Sep 05, 2018 at 08:56:49PM +0200, Christoph Hellwig wrote:
> On Wed, Sep 05, 2018 at 07:31:47AM -0700, Matthew Wilcox wrote:
> > On Wed, Sep 05, 2018 at 03:57:48PM +0200, Carlos Maiolino wrote:
> > > +	if (inode->i_op->fiemap) {
> > > +		fextent.fe_logical = 0;
> > > +		fextent.fe_physical = 0;
> > > +		fieinfo.fi_flags = FIEMAP_KERNEL_FIBMAP;
> > > +		fieinfo.fi_extents_max = 1;
> > > +		fieinfo.fi_extents_start = (__force struct fiemap_extent __user *) &fextent;
> > > +
> > > +		error = inode->i_op->fiemap(inode, &fieinfo, start, 1);
> > 
> > You'd have to play games with set_fs() and friends if you want to do this.
> > The fiemap implementation is going to access fi_extents_start with a call
> > to copy_to_user() and for machines with a 4G/4G split, you need that
> > address to be interpreted as kernel space, not user space.
> > 
> > See fiemap_fill_next_extent():
> 
> Yeah.  I think we need to pass fiemap_fill_next_extent() as a function
> pointer to fiemap in a prep patch, and then pass a different pointer
> for the in-kernel usage.  Which is good API design anyway, so we should
> have done this from the beginning.

Sorry, I'm not 100% sure I followed your point here, do you mind to detail it a
bit?
Passing fiemap_fill_next_extent() as a pointer to what? ->fiemap() interface?
Sounds interesting, but doing this looks like I'll need to do what I was trying
to avoid from the beginning, which is the creating of a second struct
fiemap_extent_info, to be used in-lernel only, so, the last field doesn't need
to be tagged as __user. I'm ok with that though, I was just trying a way to
avoid adding unneeded data structures if possible, but looks like it ended up
not being a good approach :P

Thanks a lot for the review

-- 
Carlos