On Wed, May 28, 2008 at 01:42:15PM -0600, Andreas Dilger wrote:
> On May 24, 2008 17:01 -0700, Mark Fasheh wrote:
> > Basic vfs-level fiemap infrastructure, which sets up a new ->fiemap
> > inode operation.
>
> Mark, I was looking at a way to remove the special-casing of NUM_EXTENTS
> from ioctl_fiemap() in an effort to remove Christoph's objection to
> keeping these in the same ioctl.
>
> I think it is possible and reasonable to move the special-case handling
> into fiemap_fill_next_extent().
>
> > +static int ioctl_fiemap(struct file *filp, unsigned long arg)
> > +{
> > +
> > + if (!(fiemap.fm_flags & FIEMAP_FLAG_NUM_EXTENTS) &&
> > + (fiemap.fm_extent_count == 0 ||
> > + fiemap.fm_extent_count > FIEMAP_MAX_EXTENTS))
> > + return -EINVAL;
>
> This can be changed to only check:
>
> if (fm_extent_count > FIEMAP_MAX_EXTENTS)
> return -EINVAL;
>
> > + fieinfo.fi_flags = fiemap.fm_flags;
> > + if (!(fiemap.fm_flags & FIEMAP_FLAG_NUM_EXTENTS)) {
> > + fieinfo.fi_extents_max = fiemap.fm_extent_count;
> > + fieinfo.fi_extents_start = (char *)arg + sizeof(fiemap);
> > +
> > + if (!access_ok(VERIFY_WRITE, fieinfo.fi_extents_start,
> > + fieinfo.fi_extents_max * sizeof(struct fiemap_extent)))
> > + return -EFAULT;
> > + }
>
> It is harmless to set fi_extents_max and fi_extents_start, as this is
> ignored by NUM_EXTENTS. The fiemap_fill_next_extent() will already
> check in copy_to_user() whether the fi_extents_start pointer is valid,
> and fiemap_fill_next_extent() doesn't even get far enough to look at
> fi_extents_max or fi_extents_start. We just do:
>
> fieinfo.fi_extents = fiemap.fm_extent_count;
> fieinfo.fi_extents_start = (struct fiemap_extent *)((char *)arg +
> sizeof(fiemap));
>
> This leaves us with no checks for FIEMAP_FLAG_NUM_EXTENTS in ioctl_fiemap()
> at all, and no changes needed in fiemap_fill_next_extent().
>
> > > What about the idea to have fiemap_fill_next_extent() do "extent" merging
> > > for filesystems that use the generic helper but do not return multiple
> > > blocks via get_blocks()? I don't think that is too hard to implement,
> > > and makes the output more useful, otherwise we get an extent per block.
> > > The above is what I _think_ will work, haven't actually tried it out.
> >
> > I don't think we want to automatically merge extents within this helper
> > function. Otherwise we would diverge from the actual disk layout for extent
> > based file systems where an extent might be broken up between two records
> > for some other reason, such as maximum extent length being exceeded.
>
> Do we really want to expose the filesystem-specific extent-length limits
> to userspace? In some sense, a block-based filesystem has a maximum
> extent length of the blocksize, but it seems totally reasonable to merge
> contiguous blocks into a single "extent" for return to userspace. I
> don't see this significantly different for ext4, even though it can have
> extents up to 128MB, or unwritten extents up to 64MB.
>
> > Btw, how many block-based file systems that don't return multiple blocks via
> > get_blocks() are there that we actually care about enough to write this
> > code?
>
> That I have no clue about. Joseph?
>
>
Well I can't comment on the "care enough about" part, but looking at it I don't
think reiserfs does, it just maps the one block we ask for. Just cursory
looking through stuff it looks like most other stuff does map as much as it can,
even FAT does. Thanks much,
Josef
--
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
