From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f67.google.com ([209.85.128.67]:38364 "EHLO mail-wm1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729628AbeKFSNS (ORCPT ); Tue, 6 Nov 2018 13:13:18 -0500 Received: by mail-wm1-f67.google.com with SMTP id l2-v6so10680081wmh.3 for ; Tue, 06 Nov 2018 00:49:07 -0800 (PST) Date: Tue, 6 Nov 2018 09:49:03 +0100 From: Carlos Maiolino To: Andreas Dilger Cc: Linux FS-devel Mailing List , Eric Sandeen , Christoph Hellwig , Dave Chinner , darrick.wong@oracle.com Subject: Re: [PATCH 08/20] ext4: Remove direct usage of fiemap_extent_info Message-ID: <20181106084903.coowb2a3uwox4gvg@odin.usersys.redhat.com> References: <20181030131823.29040-1-cmaiolino@redhat.com> <20181030131823.29040-9-cmaiolino@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: > > 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