[PATCH] xfs: Forbid FIEMAP on RT devices

[PATCH] xfs: Forbid FIEMAP on RT devices

[Carlos Maiolino]

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(+)

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

Re: [PATCH] xfs: Forbid FIEMAP on RT devices

[Carlos Maiolino]

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
> 

-- 
Carlos

Re: [PATCH] xfs: Forbid FIEMAP on RT devices

[Eric Sandeen]

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;
> 

Re: [PATCH] xfs: Forbid FIEMAP on RT devices

[Carlos Maiolino]

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;
> > 
> 

-- 
Carlos

Re: [PATCH] xfs: Forbid FIEMAP on RT devices

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: Forbid FIEMAP on RT devices

[Carlos Maiolino]

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

-- 
Carlos