On Jul 24, 2014, at 1:22 PM, David Sterba <dsterba@xxxxxxx> wrote: > On Thu, Jul 17, 2014 at 12:07:57AM -0600, Andreas Dilger wrote: >> any progress on this patch series? > > I'm sorry I got distracted at the end of year and did not finish the > series. > >> I never saw an updated version of this patch series after the last round of >> reviews, but it would be great to move it forward. I have filefrag patches >> in my e2fsprogs tree waiting for an updated version of your patch. >> >> I recall the main changes were: >> - add FIEMAP_EXTENT_PHYS_LENGTH flag to indicate if fe_phys_length was valid > > fe_phys_length will be always valid, so other the flags are set only if it's > not equal to the logical length. > >> - 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. 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. 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. Cheers, Andreas >> - add WARN_ONCE() in fiemap_fill_next_extent() as described below >> >> I don't know if there was any clear statement about whether there should be >> separate FIEMAP_EXTENT_PHYS_LENGTH and FIEMAP_EXTENT_DATA_COMPRESSED flags, >> or if the latter should be implicit? Probably makes sense to have separate >> flags. It should be fine to use: >> >> #define FIEMAP_EXTENT_PHYS_LENGTH 0x00000010 >> >> since this flag was never used. > > I've kept only FIEMAP_EXTENT_DATA_COMPRESSED, I don't see a need for > FIEMAP_EXTENT_PHYS_LENGTH and this would be yet another flag because the > FIEMAP_EXTENT_DATA_ENCODED is also implied. > > I'll send V4, we can discuss the PHYS_LENGTH flag then. Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail
