Re: [PATCH 08/20] ext4: Remove direct usage of fiemap_extent_info

[Carlos Maiolino <cmaiolino@redhat.com>]

> > static int ext4_fill_fiemap_extents(struct inode *inode,
> > 				    ext4_lblk_t block, ext4_lblk_t num,
> > -				    struct fiemap_extent_info *fieinfo)
> > +				    struct fiemap_ctx *f_ctx)
> > {
> > 	struct ext4_ext_path *path = NULL;
> > 	struct ext4_extent *ex;
> > @@ -2277,7 +2277,8 @@ static int ext4_fill_fiemap_extents(struct inode *inode,
> > 		}
> > 
> > 		if (exists) {
> > -			err = fiemap_fill_next_extent(fieinfo,
> > +			err = fiemap_fill_next_extent(
> > +				(struct fiemap_extent_info *)f_ctx->fc_data,
> 
> No need to cast void pointer.  This also makes me wonder why you didn't name
> fc_data as fc_extent_info instead of making it a void pointer?  I didn't see
> any users where it isn't a pointer to struct fiemap_extent_info?

On later patches, the void pointer is used to point to a userspace address or to
a kernel address, depending on the context. (userspace if FIEMAP, kernelspace if
FIBMAP).

> 
> The same is true of all the other filesystem-specific patches - there is no
> need to cast from a void *.  Is there a later user where this is used as a
> generic data pointer, or is it always going to be a fiemap_extent_info, and
> later a fiemap_ctx structure?

I believe I made the mistake to not quote the previous thread which led to this
patch, so, quoting it now:

https://www.spinics.net/lists/linux-fsdevel/msg131786.html

More specifically, I think the detailed information about the approach, was
discussed in the 2nd patch of the above RFC series:

https://www.spinics.net/lists/linux-fsdevel/msg131788.html


> 
> > @@ -5040,14 +5041,14 @@ static int ext4_xattr_fiemap(struct inode *inode,
> > 	}
> > 
> > 	if (physical)
> > -		error = fiemap_fill_next_extent(fieinfo, 0, physical,
> > -						length, flags);
> > +		error = fiemap_fill_next_extent(
> > +			(struct fiemap_extent_info *)f_ctx->fc_data,
> 
> Same...
> 

About the casts, I didn't remember if GCC allowed implicit casting when passing
it into functions (I remember variable assignment implicit cast though), so,
better safe than sorry, but I can remove it without problems in follow-up
patches.

Thanks

> > diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> > index 9c4bac18cc6c..7b9b0da60d54 100644
> > --- a/fs/ext4/inline.c
> > +++ b/fs/ext4/inline.c
> > @@ -1888,8 +1888,9 @@ int ext4_inline_data_fiemap(struct inode *inode,
> > 	physical += offsetof(struct ext4_inode, i_block);
> > 
> > 	if (physical)
> > -		error = fiemap_fill_next_extent(fieinfo, start, physical,
> > -						inline_len, flags);
> > +		error = fiemap_fill_next_extent(
> > +			(struct fiemap_extent_info *)f_ctx->fc_data,
> 
> Same...
> 
> 
> Cheers, Andreas
> 
> 
> 
> 
> 



-- 
Carlos