From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from verein.lst.de ([213.95.11.211]:50843 "EHLO newverein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728124AbeKQCRA (ORCPT ); Fri, 16 Nov 2018 21:17:00 -0500 Date: Fri, 16 Nov 2018 17:04:02 +0100 From: Christoph Hellwig To: Carlos Maiolino Cc: linux-fsdevel@vger.kernel.org, sandeen@redhat.com, hch@lst.de, david@fromorbit.com, darrick.wong@oracle.com Subject: Re: [PATCH 17/20] fiemap: Get rid of fiemap_extent_info Message-ID: <20181116160402.GA17130@lst.de> References: <20181030131823.29040-1-cmaiolino@redhat.com> <20181030131823.29040-18-cmaiolino@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181030131823.29040-18-cmaiolino@redhat.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, Oct 30, 2018 at 02:18:20PM +0100, Carlos Maiolino wrote: > With the goal of using FIEMAP infra-structure for FIBMAP ioctl, and with > the updates being done previously into the FIEMAP interface, the > fiemap_extent_info structure is no longer necessary, so, move > fi_extents_mapped and fi_extents_max up to fiemap_ctx. and use > fiemap_ctx.fc_data to hold a pointer to struct fiemap_extent. > > Signed-off-by: Carlos Maiolino > --- > fs/btrfs/extent_io.c | 3 +-- > fs/ioctl.c | 29 +++++++++++++---------------- > include/linux/fs.h | 9 ++------- > include/linux/iomap.h | 1 - > 4 files changed, 16 insertions(+), 26 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 6a8bc502bc04..88b8a4416dfc 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -4436,7 +4436,6 @@ int extent_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx) > struct btrfs_path *path; > struct btrfs_root *root = BTRFS_I(inode)->root; > struct fiemap_cache cache = { 0 }; > - struct fiemap_extent_info *fieinfo = f_ctx->fc_data; > int end = 0; > u64 em_start = 0; > u64 em_len = 0; > @@ -4561,7 +4560,7 @@ int extent_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx) > } else if (em->block_start == EXTENT_MAP_DELALLOC) { > flags |= (FIEMAP_EXTENT_DELALLOC | > FIEMAP_EXTENT_UNKNOWN); > - } else if (fieinfo->fi_extents_max) { > + } else if (f_ctx->fc_extents_max) { > u64 bytenr = em->block_start - > (em->start - em->orig_start); Shouldn't this be earlier, when or just after when we introduce f_ctx->fc_extents_max? > return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0; Can you use a plain old if statment here to make the code a little more readable? if (flags & FIEMAP_EXTENT_LAST) return 1; return 0; > struct fiemap_ctx { > unsigned int fc_flags; /* Flags as passed from user */ > + unsigned int fc_extents_mapped; /* Number of mapped extents */ > + unsigned int fc_extents_max; /* Size of fiemap_extent array */ > void *fc_data; > fiemap_fill_cb fc_cb; > u64 fc_start; This makes me wonder if we shouldn't just have kept the existing structure and massaged it into the new form. The two just look very, very similar..