diff mbox series

xfs: Forbid FIEMAP on RT devices

Message ID 20191119154453.312400-1-cmaiolino@redhat.com (mailing list archive)
State Deferred, archived
Headers show
Series xfs: Forbid FIEMAP on RT devices | expand

Commit Message

Carlos Maiolino Nov. 19, 2019, 3:44 p.m. UTC
By now, FIEMAP users have no way to identify which device contains the
mapping being reported by the ioctl, so, let's forbid FIEMAP on RT
devices/files until FIEMAP can properly report the device containing the
returned mappings.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---

Hi folks, this change has been previously suggested by Christoph while the
fibmap->fiemap work was being discussed on the last version [1] of that set.
And after some thought I do think RT devices shouldn't allow fiemap calls
either, giving the file blocks will actually be on a different device than that
displayed on /proc/mounts which can lead to erroneous assumptions.

 fs/xfs/xfs_iops.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Carlos Maiolino Nov. 19, 2019, 3:48 p.m. UTC | #1
On Tue, Nov 19, 2019 at 04:44:53PM +0100, Carlos Maiolino wrote:
> By now, FIEMAP users have no way to identify which device contains the
> mapping being reported by the ioctl, so, let's forbid FIEMAP on RT
> devices/files until FIEMAP can properly report the device containing the
> returned mappings.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> 
> Hi folks, this change has been previously suggested by Christoph while the
> fibmap->fiemap work was being discussed on the last version [1] of that set.
> And after some thought I do think RT devices shouldn't allow fiemap calls
> either, giving the file blocks will actually be on a different device than that
> displayed on /proc/mounts which can lead to erroneous assumptions.

Fingers are faster than my brain:

[1]: https://lore.kernel.org/linux-xfs/20190918132436.GA16210@lst.de/T/

> 
>  fs/xfs/xfs_iops.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index e532db27d0dc..ec7749cbd3ca 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -1138,6 +1138,9 @@ xfs_vn_fiemap(
>  {
>  	int			error;
>  
> +	if (XFS_IS_REALTIME_INODE(XFS_I(inode)))
> +		return -EOPNOTSUPP;
> +
>  	xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED);
>  	if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
>  		fieinfo->fi_flags &= ~FIEMAP_FLAG_XATTR;
> -- 
> 2.23.0
>
Eric Sandeen Nov. 19, 2019, 4:10 p.m. UTC | #2
On 11/19/19 9:44 AM, Carlos Maiolino wrote:
> By now, FIEMAP users have no way to identify which device contains the
> mapping being reported by the ioctl, so, let's forbid FIEMAP on RT
> devices/files until FIEMAP can properly report the device containing the
> returned mappings.

I'm not sure I agree with this - we don't return any device information with
any mapping, ever. It's always up to the caller to understand what they have
asked to have mapped. I'm not sure why RT files should be singled out, as long
as the mapping is correct.

-Eric

> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> 
> Hi folks, this change has been previously suggested by Christoph while the
> fibmap->fiemap work was being discussed on the last version [1] of that set.
> And after some thought I do think RT devices shouldn't allow fiemap calls
> either, giving the file blocks will actually be on a different device than that
> displayed on /proc/mounts which can lead to erroneous assumptions.
> 
>  fs/xfs/xfs_iops.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index e532db27d0dc..ec7749cbd3ca 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -1138,6 +1138,9 @@ xfs_vn_fiemap(
>  {
>  	int			error;
>  
> +	if (XFS_IS_REALTIME_INODE(XFS_I(inode)))
> +		return -EOPNOTSUPP;
> +
>  	xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED);
>  	if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
>  		fieinfo->fi_flags &= ~FIEMAP_FLAG_XATTR;
>
Carlos Maiolino Nov. 19, 2019, 4:50 p.m. UTC | #3
On Tue, Nov 19, 2019 at 10:10:54AM -0600, Eric Sandeen wrote:
> On 11/19/19 9:44 AM, Carlos Maiolino wrote:
> > By now, FIEMAP users have no way to identify which device contains the
> > mapping being reported by the ioctl, so, let's forbid FIEMAP on RT
> > devices/files until FIEMAP can properly report the device containing the
> > returned mappings.
> 
> I'm not sure I agree with this - we don't return any device information with
> any mapping, ever. It's always up to the caller to understand what they have
> asked to have mapped. I'm not sure why RT files should be singled out, as long
> as the mapping is correct.

It's been a long time discussion already on this subject, it all started back
with btrfs and their virtual address spaces and how a fiemap should behave on
filesystems such as btrfs, which, by instance, the caller can't tell which
device the mapping refers to.

XFS RT device is the simplest of this kind of case, where the mapping returned
does not match with the mounted device, but by now that's the one blocking my
FIEMAP work, which I want to fix to move on to the next steps :P

> 
> -Eric
> 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> > 
> > Hi folks, this change has been previously suggested by Christoph while the
> > fibmap->fiemap work was being discussed on the last version [1] of that set.
> > And after some thought I do think RT devices shouldn't allow fiemap calls
> > either, giving the file blocks will actually be on a different device than that
> > displayed on /proc/mounts which can lead to erroneous assumptions.
> > 
> >  fs/xfs/xfs_iops.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index e532db27d0dc..ec7749cbd3ca 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -1138,6 +1138,9 @@ xfs_vn_fiemap(
> >  {
> >  	int			error;
> >  
> > +	if (XFS_IS_REALTIME_INODE(XFS_I(inode)))
> > +		return -EOPNOTSUPP;
> > +
> >  	xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED);
> >  	if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
> >  		fieinfo->fi_flags &= ~FIEMAP_FLAG_XATTR;
> > 
>
Dave Chinner Nov. 19, 2019, 8:25 p.m. UTC | #4
On Tue, Nov 19, 2019 at 04:44:53PM +0100, Carlos Maiolino wrote:
> By now, FIEMAP users have no way to identify which device contains the
> mapping being reported by the ioctl,

This is an incorrect assertion.

Users can call FS_IOC_FSGETXATTR and look at the FS_XFLAG_REALTIME
flag returned to determine what device the XFS data mapping points
at.

> so, let's forbid FIEMAP on RT
> devices/files until FIEMAP can properly report the device containing the
> returned mappings.

Also, fiemap on the attribute fork should work regardless of where
the data for the file is located.

So that's two reasons we shouldn't do this :P

Cheers,

Dave.
Carlos Maiolino Nov. 20, 2019, 8:17 a.m. UTC | #5
On Wed, Nov 20, 2019 at 07:25:11AM +1100, Dave Chinner wrote:
> On Tue, Nov 19, 2019 at 04:44:53PM +0100, Carlos Maiolino wrote:
> > By now, FIEMAP users have no way to identify which device contains the
> > mapping being reported by the ioctl,
> 
> This is an incorrect assertion.
> 
> Users can call FS_IOC_FSGETXATTR and look at the FS_XFLAG_REALTIME
> flag returned to determine what device the XFS data mapping points
> at.

/me didn't know

> 
> > so, let's forbid FIEMAP on RT
> > devices/files until FIEMAP can properly report the device containing the
> > returned mappings.
> 
> Also, fiemap on the attribute fork should work regardless of where
> the data for the file is located.
> 

True.

> So that's two reasons we shouldn't do this :P

Fair enough.

Thanks, I'll try to find another way to deal with that :P
diff mbox series

Patch

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index e532db27d0dc..ec7749cbd3ca 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -1138,6 +1138,9 @@  xfs_vn_fiemap(
 {
 	int			error;
 
+	if (XFS_IS_REALTIME_INODE(XFS_I(inode)))
+		return -EOPNOTSUPP;
+
 	xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED);
 	if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
 		fieinfo->fi_flags &= ~FIEMAP_FLAG_XATTR;