Ext4 fiemap implementation

Ext4 fiemap implementation

[Carlos Maiolino]

Hi,

I've been working on a patchset to get rid of the ->bmap infrastructure.

FIBMAP ioctl should be supported forever, so, basically I'm using Dave's idea to
use ->fiemap() implementation to handle FIBMAP ioctl, however, I've been facing
an issue with Ext4 FIEMAP in this case; basically:

When issuing a FIEMAP ioctl to Ext4with something like this:

fiemap->fm_start = block_num * blocksize;
fiemap->fm_length = 1;
fmap->fm_extent_count = 1;

I was expecting the fiemap_extent returned, to contain the physical block
from the logical request above, so:

physical block == fiemap_ext.fe_physical / blocksize


This works on XFS, which is using iomap_fiemap infrastructure. However, it
doesn't work on Ext4.

Ext4 always returns in fiemap_ext.fe_physical, the start of the extent, and not
the offset requested initially (if it lies somewhere beyond the first block in
the extent).

Ted, is there any restriction why ext4_fiemap isn't using iomap_fiemap()? Or any
reason why ext4 fiemap always returns the offset from the beginning of the
extent? Would you oppose to have it updated to return the offset initially
requested? Or maybe, change ext4_fiemap() to use iomap_fiemap()?

I read the fiemap documentation, but I didn't get a clear understanding if
fiemap should be returning the beginning of the extent, the offset initially
requested, or if it depends on FS implementation.

Cheers.




-- 
Carlos

Re: Ext4 fiemap implementation

[Eric Sandeen]

On 6/1/18 7:36 AM, Carlos Maiolino wrote:
> Hi,
> 
> I've been working on a patchset to get rid of the ->bmap infrastructure.
> 
> FIBMAP ioctl should be supported forever, so, basically I'm using Dave's idea to
> use ->fiemap() implementation to handle FIBMAP ioctl, however, I've been facing
> an issue with Ext4 FIEMAP in this case; basically:
> 
> When issuing a FIEMAP ioctl to Ext4with something like this:
> 
> fiemap->fm_start = block_num * blocksize;
> fiemap->fm_length = 1;
> fmap->fm_extent_count = 1;
> 
> I was expecting the fiemap_extent returned, to contain the physical block
> from the logical request above, so:
> 
> physical block == fiemap_ext.fe_physical / blocksize
> 
> 
> This works on XFS, which is using iomap_fiemap infrastructure. However, it
> doesn't work on Ext4.
> 
> Ext4 always returns in fiemap_ext.fe_physical, the start of the extent, and not
> the offset requested initially (if it lies somewhere beyond the first block in
> the extent).
> 
> Ted, is there any restriction why ext4_fiemap isn't using iomap_fiemap()? Or any
> reason why ext4 fiemap always returns the offset from the beginning of the
> extent? Would you oppose to have it updated to return the offset initially
> requested? Or maybe, change ext4_fiemap() to use iomap_fiemap()?
> 
> I read the fiemap documentation, but I didn't get a clear understanding if
> fiemap should be returning the beginning of the extent, the offset initially
> requested, or if it depends on FS implementation.

I think the fiemap docs[1] explicitly state that ext4's behavior is valid:

> Extents returned mirror
> those on disk - that is, the logical offset of the 1st returned extent
> may start before fm_start, and the range covered by the last returned
> extent may end after fm_length.

so if you're going to use this, you will need to work out //your// block
based on the overlapping extent that gets returned, just like userspace would
do.

-Eric

[1] Documentation/filesystems/fiemap.txt

Re: Ext4 fiemap implementation

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2152 bytes --]

On Jun 1, 2018, at 8:36 AM, Carlos Maiolino <cmaiolino@redhat.com> wrote:
> 
> Hi,
> 
> I've been working on a patchset to get rid of the ->bmap infrastructure.
> 
> FIBMAP ioctl should be supported forever, so, basically I'm using Dave's
> idea to use ->fiemap() implementation to handle FIBMAP ioctl, however,
> I've been facing an issue with Ext4 FIEMAP in this case; basically:
> 
> When issuing a FIEMAP ioctl to Ext4with something like this:
> 
> fiemap->fm_start = block_num * blocksize;
> fiemap->fm_length = 1;
> fmap->fm_extent_count = 1;
> 
> I was expecting the fiemap_extent returned, to contain the physical block
> from the logical request above, so:
> 
> physical block == fiemap_ext.fe_physical / blocksize
> 
> 
> This works on XFS, which is using iomap_fiemap infrastructure. However, it
> doesn't work on Ext4.
> 
> Ext4 always returns in fiemap_ext.fe_physical, the start of the extent, and
> not the offset requested initially (if it lies somewhere beyond the first
> block in the extent).
> 
> Ted, is there any restriction why ext4_fiemap isn't using iomap_fiemap()?

I think the main reason is that iomap_fiemap() was _just_ added to the tree
and nobody has submitted a patch to move it over yet.

> Or any reason why ext4 fiemap always returns the offset from the beginning
> of the extent? Would you oppose to have it updated to return the offset
> initially requested? Or maybe, change ext4_fiemap() to use iomap_fiemap()?

I think both behaviours are valid.  The main benefit IMHO of returning the
start of the extent is that one doesn't have to _know_ where the extent
starts.  It is possible to "find the extent that covers byte X", which is
also true whether the byte offset is at the start or at the middle of a
block, while if it always starts at the supplied offset one has to do a
search backward to find the start of the extent.

> I read the fiemap documentation, but I didn't get a clear understanding if
> fiemap should be returning the beginning of the extent, the offset initially
> requested, or if it depends on FS implementation.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: Ext4 fiemap implementation

[Theodore Y. Ts'o]

On Fri, Jun 01, 2018 at 10:01:54AM -0500, Eric Sandeen wrote:
> > Ted, is there any restriction why ext4_fiemap isn't using iomap_fiemap()? Or any
> > reason why ext4 fiemap always returns the offset from the beginning of the
> > extent? Would you oppose to have it updated to return the offset initially
> > requested? Or maybe, change ext4_fiemap() to use iomap_fiemap()?

ext4_fiemap() predates iomap_fiemap().  In fact, it used to be that
all of the file systems had their own fiemap() implementation.   

> > I read the fiemap documentation, but I didn't get a clear understanding if
> > fiemap should be returning the beginning of the extent, the offset initially
> > requested, or if it depends on FS implementation.
> 
> I think the fiemap docs[1] explicitly state that ext4's behavior is valid:
> 
> > Extents returned mirror
> > those on disk - that is, the logical offset of the 1st returned extent
> > may start before fm_start, and the range covered by the last returned
> > extent may end after fm_length.

Actually, I read, "Extents returned mirror those on disk" as meaning
that the ext4 behavior is *mandated* by the docs.  It would be
interesting to see what XFS did before the iomap_fiemap() conversion.
Or it could have been that the docs were inconsistent with what XFS
was doing and then when when ext4_fiemap() was implemented, we
followed the docs.  Some software archeology would be required to know
for sure.

						- Ted

Re: Ext4 fiemap implementation

[Darrick J. Wong]

On Sat, Jun 02, 2018 at 11:28:53PM -0400, Theodore Y. Ts'o wrote:
> On Fri, Jun 01, 2018 at 10:01:54AM -0500, Eric Sandeen wrote:
> > > Ted, is there any restriction why ext4_fiemap isn't using iomap_fiemap()? Or any
> > > reason why ext4 fiemap always returns the offset from the beginning of the
> > > extent? Would you oppose to have it updated to return the offset initially
> > > requested? Or maybe, change ext4_fiemap() to use iomap_fiemap()?
> 
> ext4_fiemap() predates iomap_fiemap().  In fact, it used to be that
> all of the file systems had their own fiemap() implementation.   
> 
> > > I read the fiemap documentation, but I didn't get a clear understanding if
> > > fiemap should be returning the beginning of the extent, the offset initially
> > > requested, or if it depends on FS implementation.
> > 
> > I think the fiemap docs[1] explicitly state that ext4's behavior is valid:
> > 
> > > Extents returned mirror
> > > those on disk - that is, the logical offset of the 1st returned extent
> > > may start before fm_start, and the range covered by the last returned
> > > extent may end after fm_length.
> 
> Actually, I read, "Extents returned mirror those on disk" as meaning
> that the ext4 behavior is *mandated* by the docs.  It would be
> interesting to see what XFS did before the iomap_fiemap() conversion.
> Or it could have been that the docs were inconsistent with what XFS
> was doing and then when when ext4_fiemap() was implemented, we
> followed the docs.  Some software archeology would be required to know
> for sure.

IIRC the pre-iomap xfs_vn_fiemap implementation only returned extent
data for the block range requested.  As far as I can tell, the current
xfs iomap implementation retains that behavior.

The fiemap spec says that "it is valid for an extents [sic] logical
offset to start before the request or its logical length to extend past
the request".  To my eyes, that means either behavior is acceptable.

--D

> 
> 						- Ted

Re: Ext4 fiemap implementation

[Carlos Maiolino]

Sigh..

On Mon, Jun 04, 2018 at 09:43:09AM -0700, Darrick J. Wong wrote:
> On Sat, Jun 02, 2018 at 11:28:53PM -0400, Theodore Y. Ts'o wrote:
> > On Fri, Jun 01, 2018 at 10:01:54AM -0500, Eric Sandeen wrote:
> > > > Ted, is there any restriction why ext4_fiemap isn't using iomap_fiemap()? Or any
> > > > reason why ext4 fiemap always returns the offset from the beginning of the
> > > > extent? Would you oppose to have it updated to return the offset initially
> > > > requested? Or maybe, change ext4_fiemap() to use iomap_fiemap()?
> > 
> > ext4_fiemap() predates iomap_fiemap().  In fact, it used to be that
> > all of the file systems had their own fiemap() implementation.   
> > 
> > > > I read the fiemap documentation, but I didn't get a clear understanding if
> > > > fiemap should be returning the beginning of the extent, the offset initially
> > > > requested, or if it depends on FS implementation.
> > > 
> > > I think the fiemap docs[1] explicitly state that ext4's behavior is valid:
> > > 
> > > > Extents returned mirror
> > > > those on disk - that is, the logical offset of the 1st returned extent
> > > > may start before fm_start, and the range covered by the last returned
> > > > extent may end after fm_length.
> > 
> > Actually, I read, "Extents returned mirror those on disk" as meaning
> > that the ext4 behavior is *mandated* by the docs.  It would be
> > interesting to see what XFS did before the iomap_fiemap() conversion.
> > Or it could have been that the docs were inconsistent with what XFS
> > was doing and then when when ext4_fiemap() was implemented, we
> > followed the docs.  Some software archeology would be required to know
> > for sure.
> 
> IIRC the pre-iomap xfs_vn_fiemap implementation only returned extent
> data for the block range requested.  As far as I can tell, the current
> xfs iomap implementation retains that behavior.
> 
> The fiemap spec says that "it is valid for an extents [sic] logical
> offset to start before the request or its logical length to extend past
> the request".  To my eyes, that means either behavior is acceptable.
> 

Ok, thanks for the input everyone. I believe Eric's idea then is the one which
makes more sense. If both behaviors are valid, to make fiemap() usage for
fibmap, I think I'll need to get the extent returned by the filesystem and look
for the block into the extent. Thanks a lot for the ideas.


-- 
Carlos

Re: Ext4 fiemap implementation

[Darrick J. Wong]

On Wed, Jun 06, 2018 at 03:13:39PM +0200, Carlos Maiolino wrote:
> Sigh..
> 
> On Mon, Jun 04, 2018 at 09:43:09AM -0700, Darrick J. Wong wrote:
> > On Sat, Jun 02, 2018 at 11:28:53PM -0400, Theodore Y. Ts'o wrote:
> > > On Fri, Jun 01, 2018 at 10:01:54AM -0500, Eric Sandeen wrote:
> > > > > Ted, is there any restriction why ext4_fiemap isn't using iomap_fiemap()? Or any
> > > > > reason why ext4 fiemap always returns the offset from the beginning of the
> > > > > extent? Would you oppose to have it updated to return the offset initially
> > > > > requested? Or maybe, change ext4_fiemap() to use iomap_fiemap()?
> > > 
> > > ext4_fiemap() predates iomap_fiemap().  In fact, it used to be that
> > > all of the file systems had their own fiemap() implementation.   
> > > 
> > > > > I read the fiemap documentation, but I didn't get a clear understanding if
> > > > > fiemap should be returning the beginning of the extent, the offset initially
> > > > > requested, or if it depends on FS implementation.
> > > > 
> > > > I think the fiemap docs[1] explicitly state that ext4's behavior is valid:
> > > > 
> > > > > Extents returned mirror
> > > > > those on disk - that is, the logical offset of the 1st returned extent
> > > > > may start before fm_start, and the range covered by the last returned
> > > > > extent may end after fm_length.
> > > 
> > > Actually, I read, "Extents returned mirror those on disk" as meaning
> > > that the ext4 behavior is *mandated* by the docs.  It would be
> > > interesting to see what XFS did before the iomap_fiemap() conversion.
> > > Or it could have been that the docs were inconsistent with what XFS
> > > was doing and then when when ext4_fiemap() was implemented, we
> > > followed the docs.  Some software archeology would be required to know
> > > for sure.
> > 
> > IIRC the pre-iomap xfs_vn_fiemap implementation only returned extent
> > data for the block range requested.  As far as I can tell, the current
> > xfs iomap implementation retains that behavior.
> > 
> > The fiemap spec says that "it is valid for an extents [sic] logical
> > offset to start before the request or its logical length to extend past
> > the request".  To my eyes, that means either behavior is acceptable.
> > 
> 
> Ok, thanks for the input everyone. I believe Eric's idea then is the one which
> makes more sense. If both behaviors are valid, to make fiemap() usage for
> fibmap, I think I'll need to get the extent returned by the filesystem and look
> for the block into the extent. Thanks a lot for the ideas.

Just to throw another monkey wrench into the machine, have you
considered using iomap_bmap() instead?  It's new for 4.18...

--D

> 
> 
> -- 
> Carlos

Re: Ext4 fiemap implementation

[Carlos Maiolino]

On Wed, Jun 06, 2018 at 07:40:18AM -0700, Darrick J. Wong wrote:
> On Wed, Jun 06, 2018 at 03:13:39PM +0200, Carlos Maiolino wrote:
> > Sigh..
> > 
> > On Mon, Jun 04, 2018 at 09:43:09AM -0700, Darrick J. Wong wrote:
> > > On Sat, Jun 02, 2018 at 11:28:53PM -0400, Theodore Y. Ts'o wrote:
> > > > On Fri, Jun 01, 2018 at 10:01:54AM -0500, Eric Sandeen wrote:
> > > > > > Ted, is there any restriction why ext4_fiemap isn't using iomap_fiemap()? Or any
> > > > > > reason why ext4 fiemap always returns the offset from the beginning of the
> > > > > > extent? Would you oppose to have it updated to return the offset initially
> > > > > > requested? Or maybe, change ext4_fiemap() to use iomap_fiemap()?
> > > > 
> > > > ext4_fiemap() predates iomap_fiemap().  In fact, it used to be that
> > > > all of the file systems had their own fiemap() implementation.   
> > > > 
> > > > > > I read the fiemap documentation, but I didn't get a clear understanding if
> > > > > > fiemap should be returning the beginning of the extent, the offset initially
> > > > > > requested, or if it depends on FS implementation.
> > > > > 
> > > > > I think the fiemap docs[1] explicitly state that ext4's behavior is valid:
> > > > > 
> > > > > > Extents returned mirror
> > > > > > those on disk - that is, the logical offset of the 1st returned extent
> > > > > > may start before fm_start, and the range covered by the last returned
> > > > > > extent may end after fm_length.
> > > > 
> > > > Actually, I read, "Extents returned mirror those on disk" as meaning
> > > > that the ext4 behavior is *mandated* by the docs.  It would be
> > > > interesting to see what XFS did before the iomap_fiemap() conversion.
> > > > Or it could have been that the docs were inconsistent with what XFS
> > > > was doing and then when when ext4_fiemap() was implemented, we
> > > > followed the docs.  Some software archeology would be required to know
> > > > for sure.
> > > 
> > > IIRC the pre-iomap xfs_vn_fiemap implementation only returned extent
> > > data for the block range requested.  As far as I can tell, the current
> > > xfs iomap implementation retains that behavior.
> > > 
> > > The fiemap spec says that "it is valid for an extents [sic] logical
> > > offset to start before the request or its logical length to extend past
> > > the request".  To my eyes, that means either behavior is acceptable.
> > > 
> > 
> > Ok, thanks for the input everyone. I believe Eric's idea then is the one which
> > makes more sense. If both behaviors are valid, to make fiemap() usage for
> > fibmap, I think I'll need to get the extent returned by the filesystem and look
> > for the block into the extent. Thanks a lot for the ideas.
> 
> Just to throw another monkey wrench into the machine, have you
> considered using iomap_bmap() instead?  It's new for 4.18...

No, but thanks, I'll look into it.


> 
> --D
> 
> > 
> > 
> > -- 
> > Carlos

-- 
Carlos

Re: Ext4 fiemap implementation

[Darrick J. Wong]

On Thu, Jun 07, 2018 at 10:31:01AM +0200, Carlos Maiolino wrote:
> On Wed, Jun 06, 2018 at 07:40:18AM -0700, Darrick J. Wong wrote:
> > On Wed, Jun 06, 2018 at 03:13:39PM +0200, Carlos Maiolino wrote:
> > > Sigh..
> > > 
> > > On Mon, Jun 04, 2018 at 09:43:09AM -0700, Darrick J. Wong wrote:
> > > > On Sat, Jun 02, 2018 at 11:28:53PM -0400, Theodore Y. Ts'o wrote:
> > > > > On Fri, Jun 01, 2018 at 10:01:54AM -0500, Eric Sandeen wrote:
> > > > > > > Ted, is there any restriction why ext4_fiemap isn't using iomap_fiemap()? Or any
> > > > > > > reason why ext4 fiemap always returns the offset from the beginning of the
> > > > > > > extent? Would you oppose to have it updated to return the offset initially
> > > > > > > requested? Or maybe, change ext4_fiemap() to use iomap_fiemap()?
> > > > > 
> > > > > ext4_fiemap() predates iomap_fiemap().  In fact, it used to be that
> > > > > all of the file systems had their own fiemap() implementation.   
> > > > > 
> > > > > > > I read the fiemap documentation, but I didn't get a clear understanding if
> > > > > > > fiemap should be returning the beginning of the extent, the offset initially
> > > > > > > requested, or if it depends on FS implementation.
> > > > > > 
> > > > > > I think the fiemap docs[1] explicitly state that ext4's behavior is valid:
> > > > > > 
> > > > > > > Extents returned mirror
> > > > > > > those on disk - that is, the logical offset of the 1st returned extent
> > > > > > > may start before fm_start, and the range covered by the last returned
> > > > > > > extent may end after fm_length.
> > > > > 
> > > > > Actually, I read, "Extents returned mirror those on disk" as meaning
> > > > > that the ext4 behavior is *mandated* by the docs.  It would be
> > > > > interesting to see what XFS did before the iomap_fiemap() conversion.
> > > > > Or it could have been that the docs were inconsistent with what XFS
> > > > > was doing and then when when ext4_fiemap() was implemented, we
> > > > > followed the docs.  Some software archeology would be required to know
> > > > > for sure.
> > > > 
> > > > IIRC the pre-iomap xfs_vn_fiemap implementation only returned extent
> > > > data for the block range requested.  As far as I can tell, the current
> > > > xfs iomap implementation retains that behavior.
> > > > 
> > > > The fiemap spec says that "it is valid for an extents [sic] logical
> > > > offset to start before the request or its logical length to extend past
> > > > the request".  To my eyes, that means either behavior is acceptable.
> > > > 
> > > 
> > > Ok, thanks for the input everyone. I believe Eric's idea then is the one which
> > > makes more sense. If both behaviors are valid, to make fiemap() usage for
> > > fibmap, I think I'll need to get the extent returned by the filesystem and look
> > > for the block into the extent. Thanks a lot for the ideas.
> > 
> > Just to throw another monkey wrench into the machine, have you
> > considered using iomap_bmap() instead?  It's new for 4.18...
> 
> No, but thanks, I'll look into it.

You might also look at converting ext4 to use iomap_swapfile_activate
since that's new too.  iomap_bmap won't report blocks for unwritten
extents (because we shouldn't be pointing userspace at stale disk
blocks), which breaks swapfiles with unwritten extents.

--D

> 
> > 
> > --D
> > 
> > > 
> > > 
> > > -- 
> > > Carlos
> 
> -- 
> Carlos

Re: Ext4 fiemap implementation

[Carlos Maiolino]

On Thu, Jun 07, 2018 at 09:25:04AM -0700, Darrick J. Wong wrote:
> On Thu, Jun 07, 2018 at 10:31:01AM +0200, Carlos Maiolino wrote:
> > On Wed, Jun 06, 2018 at 07:40:18AM -0700, Darrick J. Wong wrote:
> > > On Wed, Jun 06, 2018 at 03:13:39PM +0200, Carlos Maiolino wrote:
> > > > Sigh..
> > > > 
> > > > On Mon, Jun 04, 2018 at 09:43:09AM -0700, Darrick J. Wong wrote:
> > > > > On Sat, Jun 02, 2018 at 11:28:53PM -0400, Theodore Y. Ts'o wrote:
> > > > > > On Fri, Jun 01, 2018 at 10:01:54AM -0500, Eric Sandeen wrote:
> > > > > > > > Ted, is there any restriction why ext4_fiemap isn't using iomap_fiemap()? Or any
> > > > > > > > reason why ext4 fiemap always returns the offset from the beginning of the
> > > > > > > > extent? Would you oppose to have it updated to return the offset initially
> > > > > > > > requested? Or maybe, change ext4_fiemap() to use iomap_fiemap()?
> > > > > > 
> > > > > > ext4_fiemap() predates iomap_fiemap().  In fact, it used to be that
> > > > > > all of the file systems had their own fiemap() implementation.   
> > > > > > 
> > > > > > > > I read the fiemap documentation, but I didn't get a clear understanding if
> > > > > > > > fiemap should be returning the beginning of the extent, the offset initially
> > > > > > > > requested, or if it depends on FS implementation.
> > > > > > > 
> > > > > > > I think the fiemap docs[1] explicitly state that ext4's behavior is valid:
> > > > > > > 
> > > > > > > > Extents returned mirror
> > > > > > > > those on disk - that is, the logical offset of the 1st returned extent
> > > > > > > > may start before fm_start, and the range covered by the last returned
> > > > > > > > extent may end after fm_length.
> > > > > > 
> > > > > > Actually, I read, "Extents returned mirror those on disk" as meaning
> > > > > > that the ext4 behavior is *mandated* by the docs.  It would be
> > > > > > interesting to see what XFS did before the iomap_fiemap() conversion.
> > > > > > Or it could have been that the docs were inconsistent with what XFS
> > > > > > was doing and then when when ext4_fiemap() was implemented, we
> > > > > > followed the docs.  Some software archeology would be required to know
> > > > > > for sure.
> > > > > 
> > > > > IIRC the pre-iomap xfs_vn_fiemap implementation only returned extent
> > > > > data for the block range requested.  As far as I can tell, the current
> > > > > xfs iomap implementation retains that behavior.
> > > > > 
> > > > > The fiemap spec says that "it is valid for an extents [sic] logical
> > > > > offset to start before the request or its logical length to extend past
> > > > > the request".  To my eyes, that means either behavior is acceptable.
> > > > > 
> > > > 
> > > > Ok, thanks for the input everyone. I believe Eric's idea then is the one which
> > > > makes more sense. If both behaviors are valid, to make fiemap() usage for
> > > > fibmap, I think I'll need to get the extent returned by the filesystem and look
> > > > for the block into the extent. Thanks a lot for the ideas.
> > > 
> > > Just to throw another monkey wrench into the machine, have you
> > > considered using iomap_bmap() instead?  It's new for 4.18...
> > 
> > No, but thanks, I'll look into it.
> 
> You might also look at converting ext4 to use iomap_swapfile_activate
> since that's new too.  iomap_bmap won't report blocks for unwritten
> extents (because we shouldn't be pointing userspace at stale disk
> blocks), which breaks swapfiles with unwritten extents.

I believe the biggest problem in converting ext4 to iomap now, is the behavior
change in ext4's FIEMAP, which goes back to my main concern, converting ext4 to
use iomap infrastructure, will make ext4 start to report the start block as the
one first requested, while current behavior will report the first block on the
extent.

Unless Ted is ok with this change, I don't think we can do it, which will
probably affect users of ext4's current behavior.


> 
> --D
> 
> > 
> > > 
> > > --D
> > > 
> > > > 
> > > > 
> > > > -- 
> > > > Carlos
> > 
> > -- 
> > Carlos

-- 
Carlos

Re: Ext4 fiemap implementation

[Mark Fasheh]

Hi Darrick,

On Mon, Jun 04, 2018 at 09:43:09AM -0700, Darrick J. Wong wrote:
> On Sat, Jun 02, 2018 at 11:28:53PM -0400, Theodore Y. Ts'o wrote:
> > On Fri, Jun 01, 2018 at 10:01:54AM -0500, Eric Sandeen wrote:
> > > > Ted, is there any restriction why ext4_fiemap isn't using iomap_fiemap()? Or any
> > > > reason why ext4 fiemap always returns the offset from the beginning of the
> > > > extent? Would you oppose to have it updated to return the offset initially
> > > > requested? Or maybe, change ext4_fiemap() to use iomap_fiemap()?
> > 
> > ext4_fiemap() predates iomap_fiemap().  In fact, it used to be that
> > all of the file systems had their own fiemap() implementation.   
> > 
> > > > I read the fiemap documentation, but I didn't get a clear understanding if
> > > > fiemap should be returning the beginning of the extent, the offset initially
> > > > requested, or if it depends on FS implementation.
> > > 
> > > I think the fiemap docs[1] explicitly state that ext4's behavior is valid:
> > > 
> > > > Extents returned mirror
> > > > those on disk - that is, the logical offset of the 1st returned extent
> > > > may start before fm_start, and the range covered by the last returned
> > > > extent may end after fm_length.
> > 
> > Actually, I read, "Extents returned mirror those on disk" as meaning
> > that the ext4 behavior is *mandated* by the docs.  It would be
> > interesting to see what XFS did before the iomap_fiemap() conversion.
> > Or it could have been that the docs were inconsistent with what XFS
> > was doing and then when when ext4_fiemap() was implemented, we
> > followed the docs.  Some software archeology would be required to know
> > for sure.
> 
> IIRC the pre-iomap xfs_vn_fiemap implementation only returned extent
> data for the block range requested.  As far as I can tell, the current
> xfs iomap implementation retains that behavior.
> 
> The fiemap spec says that "it is valid for an extents [sic] logical
> offset to start before the request or its logical length to extend past
> the request".  To my eyes, that means either behavior is acceptable.

You have to take the whole paragraph (well the first half) together:

"All offsets and lengths are in bytes and mirror those on disk.  It is valid
for an extents logical offset to start before the request or its logical
length to extend past the request."

So in other words, mirror what's on disk. That might mean that
the returned extent might have a logical start before what the user
requested. The length might be past the request too, again because we're
mirroring what's on disk.

In fact, at no point is it specified that the fs can move the logical
start of the returned extent *forward*. The text is quite explicit that the
logcal end can only be *before* the request because that's the only way that
'mirror what's on disk' can work for the user.

Thanks,
	--Mark

Btw, I realize the original e-mail was about physical offset but for the
purposes of this conversation the two values are mathematically linked.

Re: Ext4 fiemap implementation

[Carlos Maiolino]

On Sat, Jun 09, 2018 at 12:41:26AM +0200, Mark Fasheh wrote:
> Hi Darrick,
> 
> On Mon, Jun 04, 2018 at 09:43:09AM -0700, Darrick J. Wong wrote:
> > On Sat, Jun 02, 2018 at 11:28:53PM -0400, Theodore Y. Ts'o wrote:
> > > On Fri, Jun 01, 2018 at 10:01:54AM -0500, Eric Sandeen wrote:
> > > > > Ted, is there any restriction why ext4_fiemap isn't using iomap_fiemap()? Or any
> > > > > reason why ext4 fiemap always returns the offset from the beginning of the
> > > > > extent? Would you oppose to have it updated to return the offset initially
> > > > > requested? Or maybe, change ext4_fiemap() to use iomap_fiemap()?
> > > 
> > > ext4_fiemap() predates iomap_fiemap().  In fact, it used to be that
> > > all of the file systems had their own fiemap() implementation.   
> > > 
> > > > > I read the fiemap documentation, but I didn't get a clear understanding if
> > > > > fiemap should be returning the beginning of the extent, the offset initially
> > > > > requested, or if it depends on FS implementation.
> > > > 
> > > > I think the fiemap docs[1] explicitly state that ext4's behavior is valid:
> > > > 
> > > > > Extents returned mirror
> > > > > those on disk - that is, the logical offset of the 1st returned extent
> > > > > may start before fm_start, and the range covered by the last returned
> > > > > extent may end after fm_length.
> > > 
> > > Actually, I read, "Extents returned mirror those on disk" as meaning
> > > that the ext4 behavior is *mandated* by the docs.  It would be
> > > interesting to see what XFS did before the iomap_fiemap() conversion.
> > > Or it could have been that the docs were inconsistent with what XFS
> > > was doing and then when when ext4_fiemap() was implemented, we
> > > followed the docs.  Some software archeology would be required to know
> > > for sure.
> > 
> > IIRC the pre-iomap xfs_vn_fiemap implementation only returned extent
> > data for the block range requested.  As far as I can tell, the current
> > xfs iomap implementation retains that behavior.
> > 
> > The fiemap spec says that "it is valid for an extents [sic] logical
> > offset to start before the request or its logical length to extend past
> > the request".  To my eyes, that means either behavior is acceptable.
> 
> You have to take the whole paragraph (well the first half) together:
> 
> "All offsets and lengths are in bytes and mirror those on disk.  It is valid
> for an extents logical offset to start before the request or its logical
> length to extend past the request."
> 
> So in other words, mirror what's on disk. That might mean that
> the returned extent might have a logical start before what the user
> requested. The length might be past the request too, again because we're
> mirroring what's on disk.
> 
> In fact, at no point is it specified that the fs can move the logical
> start of the returned extent *forward*. The text is quite explicit that the
> logcal end can only be *before* the request because that's the only way that
> 'mirror what's on disk' can work for the user.
> 
> Thanks,
> 	--Mark
> 
> Btw, I realize the original e-mail was about physical offset but for the
> purposes of this conversation the two values are mathematically linked.

So, you are saying iomap implementation violates FIEMAP specs?

-- 
Carlos

Re: Ext4 fiemap implementation

[Mark Fasheh]

On Mon, Jun 11, 2018 at 09:28:27AM +0200, Carlos Maiolino wrote:
> On Sat, Jun 09, 2018 at 12:41:26AM +0200, Mark Fasheh wrote:
> > Hi Darrick,
> > 
> > On Mon, Jun 04, 2018 at 09:43:09AM -0700, Darrick J. Wong wrote:
> > > On Sat, Jun 02, 2018 at 11:28:53PM -0400, Theodore Y. Ts'o wrote:
> > > > On Fri, Jun 01, 2018 at 10:01:54AM -0500, Eric Sandeen wrote:
> > > > > > Ted, is there any restriction why ext4_fiemap isn't using iomap_fiemap()? Or any
> > > > > > reason why ext4 fiemap always returns the offset from the beginning of the
> > > > > > extent? Would you oppose to have it updated to return the offset initially
> > > > > > requested? Or maybe, change ext4_fiemap() to use iomap_fiemap()?
> > > > 
> > > > ext4_fiemap() predates iomap_fiemap().  In fact, it used to be that
> > > > all of the file systems had their own fiemap() implementation.   
> > > > 
> > > > > > I read the fiemap documentation, but I didn't get a clear understanding if
> > > > > > fiemap should be returning the beginning of the extent, the offset initially
> > > > > > requested, or if it depends on FS implementation.
> > > > > 
> > > > > I think the fiemap docs[1] explicitly state that ext4's behavior is valid:
> > > > > 
> > > > > > Extents returned mirror
> > > > > > those on disk - that is, the logical offset of the 1st returned extent
> > > > > > may start before fm_start, and the range covered by the last returned
> > > > > > extent may end after fm_length.
> > > > 
> > > > Actually, I read, "Extents returned mirror those on disk" as meaning
> > > > that the ext4 behavior is *mandated* by the docs.  It would be
> > > > interesting to see what XFS did before the iomap_fiemap() conversion.
> > > > Or it could have been that the docs were inconsistent with what XFS
> > > > was doing and then when when ext4_fiemap() was implemented, we
> > > > followed the docs.  Some software archeology would be required to know
> > > > for sure.
> > > 
> > > IIRC the pre-iomap xfs_vn_fiemap implementation only returned extent
> > > data for the block range requested.  As far as I can tell, the current
> > > xfs iomap implementation retains that behavior.
> > > 
> > > The fiemap spec says that "it is valid for an extents [sic] logical
> > > offset to start before the request or its logical length to extend past
> > > the request".  To my eyes, that means either behavior is acceptable.
> > 
> > You have to take the whole paragraph (well the first half) together:
> > 
> > "All offsets and lengths are in bytes and mirror those on disk.  It is valid
> > for an extents logical offset to start before the request or its logical
> > length to extend past the request."
> > 
> > So in other words, mirror what's on disk. That might mean that
> > the returned extent might have a logical start before what the user
> > requested. The length might be past the request too, again because we're
> > mirroring what's on disk.
> > 
> > In fact, at no point is it specified that the fs can move the logical
> > start of the returned extent *forward*. The text is quite explicit that the
> > logcal end can only be *before* the request because that's the only way that
> > 'mirror what's on disk' can work for the user.
> > 
> > Thanks,
> > 	--Mark
> > 
> > Btw, I realize the original e-mail was about physical offset but for the
> > purposes of this conversation the two values are mathematically linked.
> 
> So, you are saying iomap implementation violates FIEMAP specs?

Does iomap do this or just XFS? At any rate, the doc should be read as Ted
suggests: '"Extents returned mirror those on disk" as meaning that the ext4 behavior is *mandated* by the
docs.'

So anything that's mainipulating the returned extents solely to 'fit' them into a
request is wrong.
	--Mark

Re: Ext4 fiemap implementation

[Theodore Y. Ts'o]

On Wed, Jun 13, 2018 at 01:52:03AM +0200, Mark Fasheh wrote:
> > 
> > So, you are saying iomap implementation violates FIEMAP specs?
> 
> Does iomap do this or just XFS? At any rate, the doc should be read
> as Ted suggests: '"Extents returned mirror those on disk" as meaning
> that the ext4 behavior is *mandated* by the docs.'
> 
> So anything that's mainipulating the returned extents solely to
> 'fit' them into a request is wrong.

Well, or the FIEMAP specs could be changed.  If I recall correctly the
FIEMAP implementation by the various file systems predates the
documentation.  I suspect whoever wrote the docs looked at the
ext2/ext3/ext4 implementation and used that to write the
documentation.  If other file systems were doing something else, I'd
be in favor of allowing either behavior, since userspace programs who
care will need to accomodate either behavior.  Fortunately, I suspect
it matters for very few (if any) userspace programs.

	       		      	       	    - Ted

Re: Ext4 fiemap implementation

[Dave Chinner]

On Tue, Jun 12, 2018 at 11:06:01PM -0400, Theodore Y. Ts'o wrote:
> On Wed, Jun 13, 2018 at 01:52:03AM +0200, Mark Fasheh wrote:
> > > 
> > > So, you are saying iomap implementation violates FIEMAP specs?
> > 
> > Does iomap do this or just XFS? At any rate, the doc should be read
> > as Ted suggests: '"Extents returned mirror those on disk" as meaning
> > that the ext4 behavior is *mandated* by the docs.'
> > 
> > So anything that's mainipulating the returned extents solely to
> > 'fit' them into a request is wrong.
> 
> Well, or the FIEMAP specs could be changed.  If I recall correctly the
> FIEMAP implementation by the various file systems predates the
> documentation.  I suspect whoever wrote the docs looked at the
> ext2/ext3/ext4 implementation and used that to write the
> documentation.  If other file systems were doing something else, I'd
> be in favor of allowing either behavior, since userspace programs who
> care will need to accomodate either behavior.  Fortunately, I suspect
> it matters for very few (if any) userspace programs.

The horse has bolted - we can't redefine the expected behaviour as
that might break apps like cp (yes, it's still using FIEMAP for
sparse file optimisations).

IIRC, extended range reporting wasn't even intended for expanding
normal extents the way ext4 is reporting them - it was intended to
allow reporting of mappings where the calculation of exact
logical->physical mappings (i.e. trimming to the user specified
logical range) was too complex or prohibitively expensive. e.g.
compressed extents (FIEMAP_EXTENT_ENCODED) where we may know the
logical offset range of the physical extent, but we don't know the
exact mapping of logical offsets within the physical extent without
actually reading and decoding the physical extent.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: Ext4 fiemap implementation

[Theodore Y. Ts'o]

On Wed, Jun 13, 2018 at 01:32:04PM +1000, Dave Chinner wrote:
> > Well, or the FIEMAP specs could be changed.  If I recall correctly the
> > FIEMAP implementation by the various file systems predates the
> > documentation.  I suspect whoever wrote the docs looked at the
> > ext2/ext3/ext4 implementation and used that to write the
> > documentation.  If other file systems were doing something else, I'd
> > be in favor of allowing either behavior, since userspace programs who
> > care will need to accomodate either behavior.  Fortunately, I suspect
> > it matters for very few (if any) userspace programs.
> 
> The horse has bolted - we can't redefine the expected behaviour as
> that might break apps like cp (yes, it's still using FIEMAP for
> sparse file optimisations).

As far as I know cp is working with the FIEMAP behavior as implemented
by ext4 and xfs w/o any problems.  Given that ext4 and xfs have been
doing different things for a *very* long time now, in that sense the
horse has already bolted.  There may be some applications that
expecting things the way ext4 has implemnted FIEMAP (and which is
either allowed or required by the documentation -- I read it as
mandated; others think it's a "you can do it either way"); and there
may be some applications that are expecting things the way XFS has
historically implemented FIEMAP.

So my proposal was to change the docs to make it clear that Eric
Sandeen's reading (that either way is fine) is the correct
interpretation.

                                        - Ted

Re: Ext4 fiemap implementation

[Dave Chinner]

On Wed, Jun 13, 2018 at 01:04:53AM -0400, Theodore Y. Ts'o wrote:
> On Wed, Jun 13, 2018 at 01:32:04PM +1000, Dave Chinner wrote:
> > > Well, or the FIEMAP specs could be changed.  If I recall correctly the
> > > FIEMAP implementation by the various file systems predates the
> > > documentation.  I suspect whoever wrote the docs looked at the
> > > ext2/ext3/ext4 implementation and used that to write the
> > > documentation.  If other file systems were doing something else, I'd
> > > be in favor of allowing either behavior, since userspace programs who
> > > care will need to accomodate either behavior.  Fortunately, I suspect
> > > it matters for very few (if any) userspace programs.
> > 
> > The horse has bolted - we can't redefine the expected behaviour as
> > that might break apps like cp (yes, it's still using FIEMAP for
> > sparse file optimisations).
> 
> As far as I know cp is working with the FIEMAP behavior as implemented
> by ext4 and xfs w/o any problems.

It is, apart from the inherent raciness of using FIEMAP to find
holes and the fact that it won't detect cached data over unwritten
extents....

> Given that ext4 and xfs have been
> doing different things for a *very* long time now, in that sense the
> horse has already bolted.  There may be some applications that
> expecting things the way ext4 has implemnted FIEMAP (and which is
> either allowed or required by the documentation -- I read it as
> mandated; others think it's a "you can do it either way"); and there
> may be some applications that are expecting things the way XFS has
> historically implemented FIEMAP.
> 
> So my proposal was to change the docs to make it clear that Eric
> Sandeen's reading (that either way is fine) is the correct
> interpretation.

Ok, we're saying the same things - it wasn't clear to me that your
proposal was to document both behaviours as valid...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: Ext4 fiemap implementation

[Christoph Hellwig]

On Wed, Jun 13, 2018 at 05:41:50PM +1000, Dave Chinner wrote:
> > So my proposal was to change the docs to make it clear that Eric
> > Sandeen's reading (that either way is fine) is the correct
> > interpretation.
> 
> Ok, we're saying the same things - it wasn't clear to me that your
> proposal was to document both behaviours as valid...

Even if both are valid we should come to a conclusion which behavior
make more sense and switch everyone to it.

Right now we have two users of iomap_fiemap (gfs2 and xfs), and three
users of generic_block_fiemap (ext2, ext4 and hpfs), out of which two
already have iomap infrastructure and could be converted to the iomap
variant trivially, and three entirely open coded instances (f2fs, nilfs,
ocfs) which look like they could benefit a lot from using common code.

It doesn't make sense to have different implementations and different
behavior for no good reason.

Re: Ext4 fiemap implementation

[Carlos Maiolino]

On Wed, Jun 13, 2018 at 05:09:02AM -0700, Christoph Hellwig wrote:
> On Wed, Jun 13, 2018 at 05:41:50PM +1000, Dave Chinner wrote:
> > > So my proposal was to change the docs to make it clear that Eric
> > > Sandeen's reading (that either way is fine) is the correct
> > > interpretation.
> > 
> > Ok, we're saying the same things - it wasn't clear to me that your
> > proposal was to document both behaviours as valid...
> 
> Even if both are valid we should come to a conclusion which behavior
> make more sense and switch everyone to it.
> 
> Right now we have two users of iomap_fiemap (gfs2 and xfs), and three
> users of generic_block_fiemap (ext2, ext4 and hpfs), out of which two
> already have iomap infrastructure and could be converted to the iomap
> variant trivially, and three entirely open coded instances (f2fs, nilfs,
> ocfs) which look like they could benefit a lot from using common code.
> 
> It doesn't make sense to have different implementations and different
> behavior for no good reason.

Thanks, that's exactly my point, I'm working on the removal of the ->bmap
inteface, and I'd be happy if we could reach some agreement in this case ;)
Although I tend to agree that returning the beginning of the extent, makes more
sense to me, even though the required offset passed into FIEMAP is after the
beginning of the extent.

Cheers

-- 
Carlos