linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Eric Sandeen <sandeen@sandeen.net>
To: Carlos Maiolino <cmaiolino@redhat.com>,
	Eric Sandeen <sandeen@redhat.com>
Cc: linux-fsdevel@vger.kernel.org, hch@lst.de, david@fromorbit.com
Subject: Re: [PATCH 2/2] Use fiemap internal infra-structure to handle FIBMAP
Date: Thu, 6 Sep 2018 10:53:56 -0500	[thread overview]
Message-ID: <a1c0dd88-46a9-90e4-c2a8-4554cfb8b6f5@sandeen.net> (raw)
In-Reply-To: <20180906081231.3hpzuegvgy24lt6j@odin.usersys.redhat.com>

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

      reply	other threads:[~2018-09-06 20:30 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-05 13:57 [PATCH RFC 0/2] ->bmap interface retirement Carlos Maiolino
2018-09-05 13:57 ` [PATCH 1/2] fs: Replace direct ->bmap calls by bmap() Carlos Maiolino
2018-09-05 14:23   ` Eric Sandeen
2018-09-05 18:55     ` Christoph Hellwig
2018-09-06  8:04       ` Carlos Maiolino
2018-09-06  8:03     ` Carlos Maiolino
2018-09-05 13:57 ` [PATCH 2/2] Use fiemap internal infra-structure to handle FIBMAP Carlos Maiolino
2018-09-05 14:31   ` Matthew Wilcox
2018-09-05 18:56     ` Christoph Hellwig
2018-09-06  8:31       ` Carlos Maiolino
2018-09-10  7:50         ` Christoph Hellwig
2018-09-10 10:31           ` Carlos Maiolino
2018-09-26 13:34           ` Carlos Maiolino
2018-09-06  8:18     ` Carlos Maiolino
2018-09-05 14:40   ` Eric Sandeen
2018-09-06  8:12     ` Carlos Maiolino
2018-09-06 15:53       ` Eric Sandeen [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a1c0dd88-46a9-90e4-c2a8-4554cfb8b6f5@sandeen.net \
    --to=sandeen@sandeen.net \
    --cc=cmaiolino@redhat.com \
    --cc=david@fromorbit.com \
    --cc=hch@lst.de \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=sandeen@redhat.com \
    --subject='Re: [PATCH 2/2] Use fiemap internal infra-structure to handle FIBMAP' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).