From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:48104 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750909AbeCIJTC (ORCPT ); Fri, 9 Mar 2018 04:19:02 -0500 Subject: Re: [PATCH 1/2] Btrfs: fiemap: pass correct bytenr when fm_extent_count is zero To: robbieko Cc: linux-btrfs@vger.kernel.org References: <1520410819-32405-1-git-send-email-robbieko@synology.com> <1520410819-32405-2-git-send-email-robbieko@synology.com> <1de45bba-fe15-d1d6-173a-741918399d69@suse.com> <5ebe96aa-35e2-6c48-3ac9-ddf84f1d141d@suse.com> <50c1b80c55db0f334e8d3cfd54d6a3f3@synology.com> From: Nikolay Borisov Message-ID: <49a55b00-86f5-fb9c-5f05-84a7213ec4c0@suse.com> Date: Fri, 9 Mar 2018 11:18:58 +0200 MIME-Version: 1.0 In-Reply-To: <50c1b80c55db0f334e8d3cfd54d6a3f3@synology.com> Content-Type: text/plain; charset=utf-8 Sender: linux-btrfs-owner@vger.kernel.org List-ID: On 9.03.2018 11:01, robbieko wrote: > Nikolay Borisov 於 2018-03-07 19:15 寫到: >> On  7.03.2018 12:27, robbieko wrote: >>> Nikolay Borisov 於 2018-03-07 18:19 寫到: >>>> On  7.03.2018 10:20, robbieko wrote: >>>>> From: Robbie Ko >>>>> >>>>>  # mount /dev/vdb5 /mnt/btrfs >>>>>  # dd if=/dev/zero bs=16K count=4 oflag=dsync of=/mnt/btrfs/file >>>>>  # xfs_io -c "fiemap -v" /mnt/btrfs/file >>>>>  /mnt/btrfs/file: >>>>>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS >>>>>    0: [0..127]:        25088..25215       128   0x1 >>>>> >>>>> Run fiemap with fm_extent_count set to 0, we'll get wrong value 4 >>>>> instead of 1. >>>> >>>> Wrong value 4 instead of 1 for which exact column, the flags? State >>>> this >>>> explicitly. >>>> >>>> Also this seems a bit bogus since fiemap's documentation states: >>>> >>>> If fm_extent_count is zero, then the fm_extents[] array is ignored (no >>>> extents will be returned), and the fm_mapped_extents count will hold >>>> the >>>> number of extents needed in fm_extents[] to hold the file's current >>>> mapping. >>>> >>>> So when fm_extent_count we shouldn't really be returning anything from >>>> kernel. >>>> >>> >>> Sorry I did not explain clearly. >>> The value is fm_mapped_extents. >> >> But fm_mapped_extents is tagged as an OUT member, meaning the user has >> no job writing to it. >> > > > [BUG] > fm_mapped_extents is not correct when fm_extent_count is 0 > Like: >   # mount /dev/vdb5 /mnt/btrfs >   # dd if=/dev/zero bs=16K count=4 oflag=dsync of=/mnt/btrfs/file >   # xfs_io -c "fiemap -v" /mnt/btrfs/file >   /mnt/btrfs/file: >   EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS >     0: [0..127]:        25088..25215       128   0x1 > > When user space wants to get the number of file extents, > set fm_extent_count to 0 to run fiemap and then read fm_mapped_extents. > > In the above example, fiemap will return with fm_mapped_extents set to 4 , > but it should be 1 since there's only one entry in the output. This is slightly better. > > [REASON] > When fm_extent_count is 0, disko is not initialized correctly, > The value is 0 in this case, not the right bytenr. The value of what, be more explicit. " Also the problem seems to be that disko is only set if fieinfo->fi_extents_max is set. And this member is initialized, in the generic ioctl_fiemap function, to the value of used-passed fm_extent_count. So when the user passes 0 then fi_extent_max is also set to zero and this causes btrfs to not initialize disko at all. Eventually this leads emit_fiemap_extent being called with a bogus 'phys' argument preventing proper fiemap entries merging. " You see how complicated in fact the issue is, and your 2 line explanation is not really making it clear. > It will cause the fiemap merge mechanism to fail. > > [FIX] > Use correct disko. This is as uninformative as it gets... What about something like : "Move the disko initialization earlier in extent_fiemap making it independent of user-passed arguments, allowing emit_fiemap_extent to properly handle consecutive extent entries." > > Thanks. > Robbie Ko > >>> If fm_extent_count  is zero, the fm_mapped_extents count will hold the >>> number of extents needed. >>> >>> >>>> >>>>> >>>>> [REASON] >>>>> When fm_extent_count is 0, disko is not initialized correctly, >>>>> The value is 0 in this case, not the right bytenr. >>>> >>>> This is too sparse, be more explicit i.e. that disko=0 is passed to >>>> emit_fiemap_extent which then leads to issues. >>>> >>>>> >>>>> [FIX] >>>>> Use correct disko. >>>>> >>>>> Signed-off-by: Robbie Ko >>>>> --- >>>>>  fs/btrfs/extent_io.c | 4 +--- >>>>>  1 file changed, 1 insertion(+), 3 deletions(-) >>>>> >>>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >>>>> index 012d638..066b6df 100644 >>>>> --- a/fs/btrfs/extent_io.c >>>>> +++ b/fs/btrfs/extent_io.c >>>>> @@ -4567,7 +4567,7 @@ int extent_fiemap(struct inode *inode, struct >>>>> fiemap_extent_info *fieinfo, >>>>>              offset_in_extent = em_start - em->start; >>>>>          em_end = extent_map_end(em); >>>>>          em_len = em_end - em_start; >>>>> -        disko = 0; >>>>> +        disko = em->block_start + offset_in_extent; >>>>>          flags = 0; >>>>> >>>>>          /* >>>>> @@ -4590,8 +4590,6 @@ int extent_fiemap(struct inode *inode, struct >>>>> fiemap_extent_info *fieinfo, >>>>>              u64 bytenr = em->block_start - >>>>>                  (em->start - em->orig_start); >>>>> >>>>> -            disko = em->block_start + offset_in_extent; >>>>> - >>>>>              /* >>>>>               * As btrfs supports shared space, this information >>>>>               * can be exported to userspace tools via >>>>> -- >>>>> 1.9.1 >>>>> >>>>> -- >>>>> To unsubscribe from this list: send the line "unsubscribe >>>>> linux-btrfs" in >>>>> the body of a message to majordomo@vger.kernel.org >>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html >>>>> >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe >>> linux-btrfs" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html >>> > >