linux-ext4 - Re: [Ocfs2-devel] [PATCH 2/4 v3] fiemap: add EXTENT_DATA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 28 Jul 2014 18:49:09 +0200
From:	David Sterba <dsterba@...e.cz>
To:	Andreas Dilger <adilger@...ger.ca>
Cc:	dsterba@...e.cz, linux-nilfs@...r.kernel.org, xfs@....sgi.com,
	Btrfs Developer List <linux-btrfs@...r.kernel.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	Ext4 Developers List <linux-ext4@...r.kernel.org>,
	ocfs2-devel@....oracle.com
Subject: Re: [Ocfs2-devel] [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED
 flag

Thanks for the feedback.

On Thu, Jul 24, 2014 at 04:34:35PM -0600, Andreas Dilger wrote:
> >> - rename fe_length to fe_logi_length and #define fe_length fe_logi_length
> >> - always fill in fe_phys_length (= fe_logi_length for uncompressed files)
> >>  and set FIEMAP_EXTENT_PHYS_LENGTH whether the extent is compressed or not
> > 
> > This is my understanding and contradicts the first point.
> 
> I think Dave Chinner's former point was that having fe_phys_length validity
> depend on FIEMAP_EXTENT_DATA_COMPRESSED is a non-intuitive interface.  It is
> not true that fe_phys_length would always be valid, since that is not the
> case for older kernels that currently always set this field to 0, so they
> need some flag to indicate if fe_phys_length is valid.

I see the backward compatibility issue now. The patches (up to v4)
updated all filesystems to fill the physical length with logical,
but this should be 0 to keep the backward compatibility and also to keep
the changes to existing fiemap users minimal.

So for v5, PHYS_LENGTH will be introduced but only used by btrfs
together with ENCODED and COMPRESSED. Though this may look too much for
my needs to represent compressed extent, it is flexible for future use.

> Alternately,
> userspace could do:
> 
> 	if (ext->fe_phys_length == 0)
> 		ext->fe_phys_length = ext->fe_logi_length;
> 
> but that pre-supposes that fe_phys_length == 0 is never a valid value when
> fe_logi_length is non-zero, and this might introduce errors in some cases.
> I could imagine that some compression methods might not allocate any space
> at all if it was all zeroes, and just store a bit in the blockpointer or
> extent, so having a separate FIEMAP_EXTENT_PHYS_LENGTH is probably safer
> in the long run.

Sounds good to me.

> That opens up the question of whether a written zero
> filled space that gets compressed away is different from a hole, but I'd
> prefer to just return whatever the file mapping is than interpret it.

It could save some bytes, but I don' see it too practical. I expect the
amount of zeroed blocks to be low on average and the current compression
methods are able to squeeze long runs of zeros to a few tens of bytes
per page.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html