From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sandeen.net ([63.231.237.45]:59978 "EHLO sandeen.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730149AbeIFUaF (ORCPT ); Thu, 6 Sep 2018 16:30:05 -0400 Subject: Re: [PATCH 2/2] Use fiemap internal infra-structure to handle FIBMAP To: Carlos Maiolino , Eric Sandeen Cc: linux-fsdevel@vger.kernel.org, hch@lst.de, david@fromorbit.com References: <20180905135748.30098-1-cmaiolino@redhat.com> <20180905135748.30098-3-cmaiolino@redhat.com> <59c1da8a-676f-ca11-85d3-863a6f0bbf12@redhat.com> <20180906081231.3hpzuegvgy24lt6j@odin.usersys.redhat.com> From: Eric Sandeen Message-ID: Date: Thu, 6 Sep 2018 10:53:56 -0500 MIME-Version: 1.0 In-Reply-To: <20180906081231.3hpzuegvgy24lt6j@odin.usersys.redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 9/6/18 3:12 AM, Carlos Maiolino wrote: >>> + >>> + /* >>> + * Fiemap implementation of some filesystems will return the >>> + * extent map beginning at the requested offset >>> + * (fextent.fi_logical == start), while other FSes will return >>> + * the beginning of the extent at fextent.fe_logical, even if >>> + * the requested offset is somehwere in the middle of the >>> + * extent. We must be sure to return the correct block block >>> + * here in both cases. >>> + */ >>> + if (fextent.fe_logical != start) >>> + res = (fextent.fe_physical + start) >> inode->i_blkbits; >> I don't think this is correct, is it? You can't add the entire requested >> logical file offset (start) to the resulting physical extent start (fe_physical). >> >> You'd need to add the delta, (start - fextent.fe_logical) to the physical extent >> start to get the offset into the mapping, right? >> >> res = (fextent.fe_physical + >> (start - fextent.fe_logical)) >> inode->i_blkbits; > I believe you're right, yes, when I wrote the calculation above I was thinking > about "fe_logical is either 0 or equal start", and I tested it against > multi-extents files. I really have no idea how it survived my tests and xfstests > =/ Yeay... given the recent undetected FIBMAP breakage thanks to iomap conversion in xfs, it appears that we could use better test coverage of this crappy old interface. ;) -Eric