| Message ID | 20180905135748.30098-3-cmaiolino@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ->bmap interface retirement | expand |
On Wed, Sep 05, 2018 at 03:57:48PM +0200, Carlos Maiolino wrote: > + if (inode->i_op->fiemap) { > + fextent.fe_logical = 0; > + fextent.fe_physical = 0; > + fieinfo.fi_flags = FIEMAP_KERNEL_FIBMAP; > + fieinfo.fi_extents_max = 1; > + fieinfo.fi_extents_start = (__force struct fiemap_extent __user *) &fextent; > + > + error = inode->i_op->fiemap(inode, &fieinfo, start, 1); You'd have to play games with set_fs() and friends if you want to do this. The fiemap implementation is going to access fi_extents_start with a call to copy_to_user() and for machines with a 4G/4G split, you need that address to be interpreted as kernel space, not user space. See fiemap_fill_next_extent(): struct fiemap_extent __user *dest = fieinfo->fi_extents_start; ... if (copy_to_user(dest, &extent, sizeof(extent))) return -EFAULT;
On 9/5/18 8:57 AM, Carlos Maiolino wrote: > This patch modifies ioctl_fibmap to use FIEMAP interface internally. The > FIBMAP API is not changed, but now ->bmap can go. Not really, because you still call it if ->fiemap isn't there ;) > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > fs/inode.c | 38 +++++++++++++++++++++++++++++++++++++- > fs/ioctl.c | 11 +++++++++-- > include/uapi/linux/fiemap.h | 1 + > 3 files changed, 47 insertions(+), 3 deletions(-) > > diff --git a/fs/inode.c b/fs/inode.c > index 41812a3e829e..07befc5f253b 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -1588,9 +1588,45 @@ EXPORT_SYMBOL(iput); > */ > sector_t bmap(struct inode *inode, sector_t block) > { > + > + struct fiemap_extent_info fieinfo = { 0, }; > + struct fiemap_extent fextent; > + u64 start = block << inode->i_blkbits; > + int error; > sector_t res = 0; > - if (inode->i_mapping->a_ops->bmap) > + > + if (inode->i_op->fiemap) { > + fextent.fe_logical = 0; > + fextent.fe_physical = 0; > + fieinfo.fi_flags = FIEMAP_KERNEL_FIBMAP; | FIEMAP_FLAG_SYNC? > + fieinfo.fi_extents_max = 1; > + fieinfo.fi_extents_start = (__force struct fiemap_extent __user *) &fextent; > + > + error = inode->i_op->fiemap(inode, &fieinfo, start, 1); > + > + /* FIBMAP expect a zero return for error */ > + if (error) > + goto out; do you need to test fieinfo.fi_extents_mapped > 0? I don't know if it's possible to return error == 0 && fi_extents_mapped == 0, TBH. > + > + /* > + * Fiemap implementation of some filesystems will return the > + * extent map beginning at the requested offset > + * (fextent.fi_logical == start), while other FSes will return > + * the beginning of the extent at fextent.fe_logical, even if > + * the requested offset is somehwere in the middle of the > + * extent. We must be sure to return the correct block block > + * here in both cases. > + */ > + if (fextent.fe_logical != start) > + res = (fextent.fe_physical + start) >> inode->i_blkbits; I don't think this is correct, is it? You can't add the entire requested logical file offset (start) to the resulting physical extent start (fe_physical). You'd need to add the delta, (start - fextent.fe_logical) to the physical extent start to get the offset into the mapping, right? res = (fextent.fe_physical + (start - fextent.fe_logical)) >> inode->i_blkbits; You'll also need to test the resulting fm_flags to be sure that this is something that's valid to return via FIBMAP. For example, you might get a shared block, which we have recently learned Must Not Be Provided Under Any Circumstances to userspace via FIBMAP. (I'm still thinking about how this change is structured in general, but I think the above are a few logical issues w/ the code.) -Eric > + else > + res = fextent.fe_physical >> inode->i_blkbits; > + > + } else if (inode->i_mapping->a_ops->bmap) { > res = inode->i_mapping->a_ops->bmap(inode->i_mapping, block); > + } > + > +out: > return res; > } > EXPORT_SYMBOL(bmap); > diff --git a/fs/ioctl.c b/fs/ioctl.c > index 413585d58415..58de1dd507b1 100644 > --- a/fs/ioctl.c > +++ b/fs/ioctl.c > @@ -117,8 +117,14 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical, > extent.fe_flags = flags; > > dest += fieinfo->fi_extents_mapped; > - if (copy_to_user(dest, &extent, sizeof(extent))) > - return -EFAULT; > + > + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP) { > + memcpy((__force struct fiemap_extent *)dest, &extent, > + sizeof(extent)); > + } else { > + if (copy_to_user(dest, &extent, sizeof(extent))) > + return -EFAULT; > + } > > fieinfo->fi_extents_mapped++; > if (fieinfo->fi_extents_mapped == fieinfo->fi_extents_max) > @@ -146,6 +152,7 @@ int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags) > u32 incompat_flags; > > incompat_flags = fieinfo->fi_flags & ~(FIEMAP_FLAGS_COMPAT & fs_flags); > + incompat_flags &= ~(FIEMAP_KERNEL_FIBMAP); > if (incompat_flags) { > fieinfo->fi_flags = incompat_flags; > return -EBADR; > diff --git a/include/uapi/linux/fiemap.h b/include/uapi/linux/fiemap.h > index 8c0bc24d5d95..7064a3fa5421 100644 > --- a/include/uapi/linux/fiemap.h > +++ b/include/uapi/linux/fiemap.h > @@ -42,6 +42,7 @@ struct fiemap { > #define FIEMAP_FLAG_SYNC 0x00000001 /* sync file data before map */ > #define FIEMAP_FLAG_XATTR 0x00000002 /* map extended attribute tree */ > #define FIEMAP_FLAG_CACHE 0x00000004 /* request caching of the extents */ > +#define FIEMAP_KERNEL_FIBMAP 0x00000008 /* Use FIEMAP for FIBMAP */ > > #define FIEMAP_FLAGS_COMPAT (FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR) > >
On Wed, Sep 05, 2018 at 07:31:47AM -0700, Matthew Wilcox wrote: > On Wed, Sep 05, 2018 at 03:57:48PM +0200, Carlos Maiolino wrote: > > + if (inode->i_op->fiemap) { > > + fextent.fe_logical = 0; > > + fextent.fe_physical = 0; > > + fieinfo.fi_flags = FIEMAP_KERNEL_FIBMAP; > > + fieinfo.fi_extents_max = 1; > > + fieinfo.fi_extents_start = (__force struct fiemap_extent __user *) &fextent; > > + > > + error = inode->i_op->fiemap(inode, &fieinfo, start, 1); > > You'd have to play games with set_fs() and friends if you want to do this. > The fiemap implementation is going to access fi_extents_start with a call > to copy_to_user() and for machines with a 4G/4G split, you need that > address to be interpreted as kernel space, not user space. > > See fiemap_fill_next_extent(): Yeah. I think we need to pass fiemap_fill_next_extent() as a function pointer to fiemap in a prep patch, and then pass a different pointer for the in-kernel usage. Which is good API design anyway, so we should have done this from the beginning.
On Wed, Sep 05, 2018 at 09:40:06AM -0500, Eric Sandeen wrote: > On 9/5/18 8:57 AM, Carlos Maiolino wrote: > > This patch modifies ioctl_fibmap to use FIEMAP interface internally. The > > FIBMAP API is not changed, but now ->bmap can go. > > Not really, because you still call it if ->fiemap isn't there ;) Not sure if I understand what you meant here :( What I meant to say is that, FIBMAP behavior from user's perspective isn't supposed to change, neither the user API which needs to be used (we should support FIBMAP forever, and breaking user's API is a big NO here). Not sure if I didn't understand what you meant here, or maybe we are talking about different things? =/ > > > > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > > --- > > fs/inode.c | 38 +++++++++++++++++++++++++++++++++++++- > > fs/ioctl.c | 11 +++++++++-- > > include/uapi/linux/fiemap.h | 1 + > > 3 files changed, 47 insertions(+), 3 deletions(-) > > > > diff --git a/fs/inode.c b/fs/inode.c > > index 41812a3e829e..07befc5f253b 100644 > > --- a/fs/inode.c > > +++ b/fs/inode.c > > @@ -1588,9 +1588,45 @@ EXPORT_SYMBOL(iput); > > */ > > sector_t bmap(struct inode *inode, sector_t block) > > { > > + > > + struct fiemap_extent_info fieinfo = { 0, }; > > + struct fiemap_extent fextent; > > + u64 start = block << inode->i_blkbits; > > + int error; > > sector_t res = 0; > > - if (inode->i_mapping->a_ops->bmap) > > + > > + if (inode->i_op->fiemap) { > > + fextent.fe_logical = 0; > > + fextent.fe_physical = 0; > > + fieinfo.fi_flags = FIEMAP_KERNEL_FIBMAP; > > | FIEMAP_FLAG_SYNC? > > > + fieinfo.fi_extents_max = 1; > > + fieinfo.fi_extents_start = (__force struct fiemap_extent __user *) &fextent; > > + > > + error = inode->i_op->fiemap(inode, &fieinfo, start, 1); > > + > > + /* FIBMAP expect a zero return for error */ > > + if (error) > > + goto out; > > do you need to test fieinfo.fi_extents_mapped > 0? I don't know if it's > possible to return error == 0 && fi_extents_mapped == 0, TBH. > > > + > > + /* > > + * Fiemap implementation of some filesystems will return the > > + * extent map beginning at the requested offset > > + * (fextent.fi_logical == start), while other FSes will return > > + * the beginning of the extent at fextent.fe_logical, even if > > + * the requested offset is somehwere in the middle of the > > + * extent. We must be sure to return the correct block block > > + * here in both cases. > > + */ > > + if (fextent.fe_logical != start) > > + res = (fextent.fe_physical + start) >> inode->i_blkbits; > > I don't think this is correct, is it? You can't add the entire requested > logical file offset (start) to the resulting physical extent start (fe_physical). > > You'd need to add the delta, (start - fextent.fe_logical) to the physical extent > start to get the offset into the mapping, right? > > res = (fextent.fe_physical + > (start - fextent.fe_logical)) >> inode->i_blkbits; I believe you're right, yes, when I wrote the calculation above I was thinking about "fe_logical is either 0 or equal start", and I tested it against multi-extents files. I really have no idea how it survived my tests and xfstests =/ > > You'll also need to test the resulting fm_flags to be sure that > this is something that's valid to return via FIBMAP. > > For example, you might get a shared block, which we have recently learned > Must Not Be Provided Under Any Circumstances to userspace via FIBMAP. Right, good point. > > (I'm still thinking about how this change is structured in general, but I think > the above are a few logical issues w/ the code.) > Yes, makes sense, and I'm still also thinking on a way to better structure it. And that's the point I decided to send an RFC, better to start a conversation with a somehow working patch. Thanks for the input, much appreciated. > -Eric > > > + else > > + res = fextent.fe_physical >> inode->i_blkbits; > > + > > + } else if (inode->i_mapping->a_ops->bmap) { > > res = inode->i_mapping->a_ops->bmap(inode->i_mapping, block); > > + } > > + > > +out: > > return res; > > } > > EXPORT_SYMBOL(bmap); > > diff --git a/fs/ioctl.c b/fs/ioctl.c > > index 413585d58415..58de1dd507b1 100644 > > --- a/fs/ioctl.c > > +++ b/fs/ioctl.c > > @@ -117,8 +117,14 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical, > > extent.fe_flags = flags; > > > > dest += fieinfo->fi_extents_mapped; > > - if (copy_to_user(dest, &extent, sizeof(extent))) > > - return -EFAULT; > > + > > + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP) { > > + memcpy((__force struct fiemap_extent *)dest, &extent, > > + sizeof(extent)); > > + } else { > > + if (copy_to_user(dest, &extent, sizeof(extent))) > > + return -EFAULT; > > + } > > > > fieinfo->fi_extents_mapped++; > > if (fieinfo->fi_extents_mapped == fieinfo->fi_extents_max) > > @@ -146,6 +152,7 @@ int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags) > > u32 incompat_flags; > > > > incompat_flags = fieinfo->fi_flags & ~(FIEMAP_FLAGS_COMPAT & fs_flags); > > + incompat_flags &= ~(FIEMAP_KERNEL_FIBMAP); > > if (incompat_flags) { > > fieinfo->fi_flags = incompat_flags; > > return -EBADR; > > diff --git a/include/uapi/linux/fiemap.h b/include/uapi/linux/fiemap.h > > index 8c0bc24d5d95..7064a3fa5421 100644 > > --- a/include/uapi/linux/fiemap.h > > +++ b/include/uapi/linux/fiemap.h > > @@ -42,6 +42,7 @@ struct fiemap { > > #define FIEMAP_FLAG_SYNC 0x00000001 /* sync file data before map */ > > #define FIEMAP_FLAG_XATTR 0x00000002 /* map extended attribute tree */ > > #define FIEMAP_FLAG_CACHE 0x00000004 /* request caching of the extents */ > > +#define FIEMAP_KERNEL_FIBMAP 0x00000008 /* Use FIEMAP for FIBMAP */ > > > > #define FIEMAP_FLAGS_COMPAT (FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR) > > > > >
Hi Matthew. On Wed, Sep 05, 2018 at 07:31:47AM -0700, Matthew Wilcox wrote: > On Wed, Sep 05, 2018 at 03:57:48PM +0200, Carlos Maiolino wrote: > > + if (inode->i_op->fiemap) { > > + fextent.fe_logical = 0; > > + fextent.fe_physical = 0; > > + fieinfo.fi_flags = FIEMAP_KERNEL_FIBMAP; > > + fieinfo.fi_extents_max = 1; > > + fieinfo.fi_extents_start = (__force struct fiemap_extent __user *) &fextent; > > + > > + error = inode->i_op->fiemap(inode, &fieinfo, start, 1); > > You'd have to play games with set_fs() and friends if you want to do this. > The fiemap implementation is going to access fi_extents_start with a call > to copy_to_user() and for machines with a 4G/4G split, you need that > address to be interpreted as kernel space, not user space. > Thanks for the review, It's the very first time I try to 'mix' __user pointers with internal-only kernel code, I did some research on how other parts of kernel handles it, so I'm kind of stepping on eggs here. I do have a question about your point though: > See fiemap_fill_next_extent(): > > struct fiemap_extent __user *dest = fieinfo->fi_extents_start; > ... > if (copy_to_user(dest, &extent, sizeof(extent))) > return -EFAULT; > Yes, I noticed it, and, that's why I added the FIEMAP_KERNEL_FIBMAP flag. This is the changes I did to fiemap_fill_next_extent(): @@ -117,8 +117,14 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical, extent.fe_flags = flags; dest += fieinfo->fi_extents_mapped; - if (copy_to_user(dest, &extent, sizeof(extent))) - return -EFAULT; + + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP) { + memcpy((__force struct fiemap_extent *)dest, &extent, + sizeof(extent)); + } else { + if (copy_to_user(dest, &extent, sizeof(extent))) + return -EFAULT; + } So, my patch is going to use memcpy() directly, instead of copy_to_user(), if the request comes from the bmap() function, other than that, I'm not really sure I understand your point here.
Hi Christoph, On Wed, Sep 05, 2018 at 08:56:49PM +0200, Christoph Hellwig wrote: > On Wed, Sep 05, 2018 at 07:31:47AM -0700, Matthew Wilcox wrote: > > On Wed, Sep 05, 2018 at 03:57:48PM +0200, Carlos Maiolino wrote: > > > + if (inode->i_op->fiemap) { > > > + fextent.fe_logical = 0; > > > + fextent.fe_physical = 0; > > > + fieinfo.fi_flags = FIEMAP_KERNEL_FIBMAP; > > > + fieinfo.fi_extents_max = 1; > > > + fieinfo.fi_extents_start = (__force struct fiemap_extent __user *) &fextent; > > > + > > > + error = inode->i_op->fiemap(inode, &fieinfo, start, 1); > > > > You'd have to play games with set_fs() and friends if you want to do this. > > The fiemap implementation is going to access fi_extents_start with a call > > to copy_to_user() and for machines with a 4G/4G split, you need that > > address to be interpreted as kernel space, not user space. > > > > See fiemap_fill_next_extent(): > > Yeah. I think we need to pass fiemap_fill_next_extent() as a function > pointer to fiemap in a prep patch, and then pass a different pointer > for the in-kernel usage. Which is good API design anyway, so we should > have done this from the beginning. Sorry, I'm not 100% sure I followed your point here, do you mind to detail it a bit? Passing fiemap_fill_next_extent() as a pointer to what? ->fiemap() interface? Sounds interesting, but doing this looks like I'll need to do what I was trying to avoid from the beginning, which is the creating of a second struct fiemap_extent_info, to be used in-lernel only, so, the last field doesn't need to be tagged as __user. I'm ok with that though, I was just trying a way to avoid adding unneeded data structures if possible, but looks like it ended up not being a good approach :P Thanks a lot for the review
On 9/6/18 3:12 AM, Carlos Maiolino wrote: >>> + >>> + /* >>> + * Fiemap implementation of some filesystems will return the >>> + * extent map beginning at the requested offset >>> + * (fextent.fi_logical == start), while other FSes will return >>> + * the beginning of the extent at fextent.fe_logical, even if >>> + * the requested offset is somehwere in the middle of the >>> + * extent. We must be sure to return the correct block block >>> + * here in both cases. >>> + */ >>> + if (fextent.fe_logical != start) >>> + res = (fextent.fe_physical + start) >> inode->i_blkbits; >> I don't think this is correct, is it? You can't add the entire requested >> logical file offset (start) to the resulting physical extent start (fe_physical). >> >> You'd need to add the delta, (start - fextent.fe_logical) to the physical extent >> start to get the offset into the mapping, right? >> >> res = (fextent.fe_physical + >> (start - fextent.fe_logical)) >> inode->i_blkbits; > I believe you're right, yes, when I wrote the calculation above I was thinking > about "fe_logical is either 0 or equal start", and I tested it against > multi-extents files. I really have no idea how it survived my tests and xfstests > =/ Yeay... given the recent undetected FIBMAP breakage thanks to iomap conversion in xfs, it appears that we could use better test coverage of this crappy old interface. ;) -Eric
On Thu, Sep 06, 2018 at 10:31:05AM +0200, Carlos Maiolino wrote: > > Sorry, I'm not 100% sure I followed your point here, do you mind to detail it a > bit? > Passing fiemap_fill_next_extent() as a pointer to what? ->fiemap() interface? Yes. > Sounds interesting, but doing this looks like I'll need to do what I was trying > to avoid from the beginning, which is the creating of a second struct > fiemap_extent_info, to be used in-lernel only, so, the last field doesn't need > to be tagged as __user. I'm ok with that though, I was just trying a way to > avoid adding unneeded data structures if possible, but looks like it ended up > not being a good approach :P fieinfo is mostly used as an opaqueue cookie, so I think it should be possible to just pass a void pointer to fiemap and only let the 'filler' callback (that is fiemap_fill_next_extent or whatever a kernel caller passed) interpret it. The only thing breaking this right now seems to be fi_flags, so maybe we just need to pass that explicitly as another argument to ->fiemap. Something like: typedef int (fiemap_fill_cb)(struct inode *inode, void *data, u64 logical, u64 phys, u64 len, u32 flags); struct file_operations { ... int (*fiemap)(struct inode *, unsigned int, fiemap_fill_cb, void *, u64 start, u64 len); ... };
On Mon, Sep 10, 2018 at 09:50:39AM +0200, Christoph Hellwig wrote: > On Thu, Sep 06, 2018 at 10:31:05AM +0200, Carlos Maiolino wrote: > > > > Sorry, I'm not 100% sure I followed your point here, do you mind to detail it a > > bit? > > Passing fiemap_fill_next_extent() as a pointer to what? ->fiemap() interface? > > Yes. > > > Sounds interesting, but doing this looks like I'll need to do what I was trying > > to avoid from the beginning, which is the creating of a second struct > > fiemap_extent_info, to be used in-lernel only, so, the last field doesn't need > > to be tagged as __user. I'm ok with that though, I was just trying a way to > > avoid adding unneeded data structures if possible, but looks like it ended up > > not being a good approach :P > > fieinfo is mostly used as an opaqueue cookie, so I think it should > be possible to just pass a void pointer to fiemap and only let the 'filler' > callback (that is fiemap_fill_next_extent or whatever a kernel caller > passed) interpret it. > > The only thing breaking this right now seems to be fi_flags, so maybe > we just need to pass that explicitly as another argument to ->fiemap. > > Something like: > > typedef int (fiemap_fill_cb)(struct inode *inode, void *data, u64 logical, > u64 phys, u64 len, u32 flags); > > struct file_operations { > ... > int (*fiemap)(struct inode *, unsigned int, fiemap_fill_cb, > void *, u64 start, u64 len); > ... Thanks, this gives me some extra information to keep working on it. And I agree this is a good way to refactor this infra-structure. I'll work on something based on this and send it out to the list. > };
Hi Christoph. On Mon, Sep 10, 2018 at 09:50:39AM +0200, Christoph Hellwig wrote: > On Thu, Sep 06, 2018 at 10:31:05AM +0200, Carlos Maiolino wrote: > > > > Sorry, I'm not 100% sure I followed your point here, do you mind to detail it a > > bit? > > Passing fiemap_fill_next_extent() as a pointer to what? ->fiemap() interface? > > Yes. > > > Sounds interesting, but doing this looks like I'll need to do what I was trying > > to avoid from the beginning, which is the creating of a second struct > > fiemap_extent_info, to be used in-lernel only, so, the last field doesn't need > > to be tagged as __user. I'm ok with that though, I was just trying a way to > > avoid adding unneeded data structures if possible, but looks like it ended up > > not being a good approach :P > > fieinfo is mostly used as an opaqueue cookie, so I think it should > be possible to just pass a void pointer to fiemap and only let the 'filler' > callback (that is fiemap_fill_next_extent or whatever a kernel caller > passed) interpret it. > > The only thing breaking this right now seems to be fi_flags, so maybe > we just need to pass that explicitly as another argument to ->fiemap. > > Something like: > > typedef int (fiemap_fill_cb)(struct inode *inode, void *data, u64 logical, > u64 phys, u64 len, u32 flags); > > struct file_operations { > ... > int (*fiemap)(struct inode *, unsigned int, fiemap_fill_cb, > void *, u64 start, u64 len); > ... > }; I've been playing around with this idea, and one thing that I've been having problems with (at least that I'm not liking the end result), is the fact some filesystems keep juggling with the *fiemap_extent_info passed to them. Like ext4, for example, where it takes the pointer and passes it to its internal functions or to generic_block_fiemap if that's the case. I was wondering if, maybe, creating a new structure to embed the cookie, the flags, and the fill callback wouldn't be better, I've been thinking it might be easier to refactor the code needed to handle this new structure with all needed data into it, instead of handling the callback, flags and the cookie by their own. Maybe even create some helpers to handle the data into it. What you think?
diff --git a/fs/inode.c b/fs/inode.c index 41812a3e829e..07befc5f253b 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -1588,9 +1588,45 @@ EXPORT_SYMBOL(iput); */ sector_t bmap(struct inode *inode, sector_t block) { + + struct fiemap_extent_info fieinfo = { 0, }; + struct fiemap_extent fextent; + u64 start = block << inode->i_blkbits; + int error; sector_t res = 0; - if (inode->i_mapping->a_ops->bmap) + + if (inode->i_op->fiemap) { + fextent.fe_logical = 0; + fextent.fe_physical = 0; + fieinfo.fi_flags = FIEMAP_KERNEL_FIBMAP; + fieinfo.fi_extents_max = 1; + fieinfo.fi_extents_start = (__force struct fiemap_extent __user *) &fextent; + + error = inode->i_op->fiemap(inode, &fieinfo, start, 1); + + /* FIBMAP expect a zero return for error */ + if (error) + goto out; + + /* + * Fiemap implementation of some filesystems will return the + * extent map beginning at the requested offset + * (fextent.fi_logical == start), while other FSes will return + * the beginning of the extent at fextent.fe_logical, even if + * the requested offset is somehwere in the middle of the + * extent. We must be sure to return the correct block block + * here in both cases. + */ + if (fextent.fe_logical != start) + res = (fextent.fe_physical + start) >> inode->i_blkbits; + else + res = fextent.fe_physical >> inode->i_blkbits; + + } else if (inode->i_mapping->a_ops->bmap) { res = inode->i_mapping->a_ops->bmap(inode->i_mapping, block); + } + +out: return res; } EXPORT_SYMBOL(bmap); diff --git a/fs/ioctl.c b/fs/ioctl.c index 413585d58415..58de1dd507b1 100644 --- a/fs/ioctl.c +++ b/fs/ioctl.c @@ -117,8 +117,14 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical, extent.fe_flags = flags; dest += fieinfo->fi_extents_mapped; - if (copy_to_user(dest, &extent, sizeof(extent))) - return -EFAULT; + + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP) { + memcpy((__force struct fiemap_extent *)dest, &extent, + sizeof(extent)); + } else { + if (copy_to_user(dest, &extent, sizeof(extent))) + return -EFAULT; + } fieinfo->fi_extents_mapped++; if (fieinfo->fi_extents_mapped == fieinfo->fi_extents_max) @@ -146,6 +152,7 @@ int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags) u32 incompat_flags; incompat_flags = fieinfo->fi_flags & ~(FIEMAP_FLAGS_COMPAT & fs_flags); + incompat_flags &= ~(FIEMAP_KERNEL_FIBMAP); if (incompat_flags) { fieinfo->fi_flags = incompat_flags; return -EBADR; diff --git a/include/uapi/linux/fiemap.h b/include/uapi/linux/fiemap.h index 8c0bc24d5d95..7064a3fa5421 100644 --- a/include/uapi/linux/fiemap.h +++ b/include/uapi/linux/fiemap.h @@ -42,6 +42,7 @@ struct fiemap { #define FIEMAP_FLAG_SYNC 0x00000001 /* sync file data before map */ #define FIEMAP_FLAG_XATTR 0x00000002 /* map extended attribute tree */ #define FIEMAP_FLAG_CACHE 0x00000004 /* request caching of the extents */ +#define FIEMAP_KERNEL_FIBMAP 0x00000008 /* Use FIEMAP for FIBMAP */ #define FIEMAP_FLAGS_COMPAT (FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR)
This patch modifies ioctl_fibmap to use FIEMAP interface internally. The FIBMAP API is not changed, but now ->bmap can go. Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> --- fs/inode.c | 38 +++++++++++++++++++++++++++++++++++++- fs/ioctl.c | 11 +++++++++-- include/uapi/linux/fiemap.h | 1 + 3 files changed, 47 insertions(+), 3 deletions(-)