On 2013-10-02, at 9:02 AM, David Sterba wrote: > The original FIEMAP patch did not define this bit, btrfs will make use of > it. The defined constant maintains the same value as originally proposed. > > Currently, the 'filefrag' utility has no way to recognize and denote a > compressed extent. As implemented in btrfs right now, the compression step > splits a big extent into smaller chunks and this is reported as a heavily > fragmented file. Adding the flag to filefrag will at least give some > explanation why, this has been confusing users for some time already. > > V2: > Based on feedback from Andreas, the fiemap_extent is now able to hold the > physical extent length, to be filled by the filesystem callback. Thanks for this update. > The filesystems do not have access to the structure that is passed back to > userspace and are supposed to call fiemap_fill_next_extent, there's no > way to fill fe_phys_length. There are two ways to pass it: > > 1) extend fiemap_fill_next_extent to take phys_length and update all > users (ext4, gfs2, ocfs2, nilfs2, xfs) > > 2) add new function that takes arguments for all the fiemap_extent items, > newly added phys_length compared to fiemap_fill_next_extent > > This patchset implements 2, the footprint is smaller, no need to change > filesystem that do not care about compression. The fiemap_fill_next_extent_full() doesn't really take "all" of the fields either - it just adds the one new phys_len field, but will again be similarly lacking the next time a new field is used in struct fiemap_extent. For example, I've been wanting to add a 32-bit fm_device field in place of fe_reserved[0] so that filesystems that handle multiple devices directly (e.g. btrfs with internal RAID, xfs with "realtime" volume, ext4 with external journal, lustre, etc) could disambiguate extents between their backing devices. I think in the fiemap_fill_next_extent() will just end up having all of the fiemap_extent fields as arguments, and the callers may as well just fill in an on-stack struct fiemap_extent and pass that as a single pointer instead of passing 64 bytes of arguments and then fiemap_fill_next_extent() allocates its own struct fiemap_extent on the stack anyway. So, I don't think it is worthwhile to introduce the "_full" version of this function unless it is a short-term convenience for the sake of this patch series, and it will be removed after converting the other in-kernel users over to calling it. The change is totally trivial - just pass "0" for the new argument. Keeping fiemap_fill_next_extent() unchanged just adds 40+ bytes of stack usage to all of the existing callers for no real benefit. Cheers, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
