[PATCH 0/2] xfs: cleanup XFS_IOC_SETXATTR behaviour

[PATCH 0/2] xfs: cleanup XFS_IOC_SETXATTR behaviour

[Dave Chinner]

Hi folks,

A while back Iustin Pop sent a patch to fix a problem with being
unable to set extent size hint values on directories. That patch -
along with new xfstests functionality to always check the scratch
device after a test - has pointed out that we allow certain
directory only inode flags to be set on other types of inodes (e.g.
regular files). It also pointed out that we could set extent size
hints on inodes that don't have extent size hint flags set.

This patchset does not attempt to fix the original problem, not add
any new validation of what is passed from userspace. Instead, it
simply ensures that what we end up with on disk is valid. That is,
directory only flags are only set on directory inodes, and extent
size hints are set if the inode flags are set, otherwise it is
cleared.  Hence we don't end up with "invalid" state on disk, and so
xfstests doesn't get upset with directory only flags being set on
non directory inodes.

Further followups will be needed to address the original issue of
changing extent size hints on directories and handling invalid
flag/value combinations from userspace. I have not attempted to
solve that problem here because it could have impact on userspace
application behaviour and that's a different issue to ensuring we
end up with valid inode state on disk.

Thoughts, comments, flames?

Cheers,

Dave.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/2] xfs: project id inheritance is a directory only flag

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

xfs_set_diflags() allows it to be set on non-directory inodes, and
this flags errors in xfs_repair. Further, inode allocation allows
the same directory-only flag to be inherited to non-directories.
Make sure directory inode flags don't appear on other types of
inodes.

This fixes several xfstests scratch fileystem corruption reports
(e.g. xfs/050) now that xfstests checks scratch filesystems after
test completion.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_inode.c | 4 ++--
 fs/xfs/xfs_ioctl.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index f07b443..8ed049d 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -769,6 +769,8 @@ xfs_ialloc(
 					di_flags |= XFS_DIFLAG_EXTSZINHERIT;
 					ip->i_d.di_extsize = pip->i_d.di_extsize;
 				}
+				if (pip->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)
+					di_flags |= XFS_DIFLAG_PROJINHERIT;
 			} else if (S_ISREG(mode)) {
 				if (pip->i_d.di_flags & XFS_DIFLAG_RTINHERIT)
 					di_flags |= XFS_DIFLAG_REALTIME;
@@ -789,8 +791,6 @@ xfs_ialloc(
 			if ((pip->i_d.di_flags & XFS_DIFLAG_NOSYMLINKS) &&
 			    xfs_inherit_nosymlinks)
 				di_flags |= XFS_DIFLAG_NOSYMLINKS;
-			if (pip->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)
-				di_flags |= XFS_DIFLAG_PROJINHERIT;
 			if ((pip->i_d.di_flags & XFS_DIFLAG_NODEFRAG) &&
 			    xfs_inherit_nodefrag)
 				di_flags |= XFS_DIFLAG_NODEFRAG;
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 7a6b406..87c3bd1 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -968,8 +968,6 @@ xfs_set_diflags(
 		di_flags |= XFS_DIFLAG_NOATIME;
 	if (xflags & XFS_XFLAG_NODUMP)
 		di_flags |= XFS_DIFLAG_NODUMP;
-	if (xflags & XFS_XFLAG_PROJINHERIT)
-		di_flags |= XFS_DIFLAG_PROJINHERIT;
 	if (xflags & XFS_XFLAG_NODEFRAG)
 		di_flags |= XFS_DIFLAG_NODEFRAG;
 	if (xflags & XFS_XFLAG_FILESTREAM)
@@ -981,6 +979,8 @@ xfs_set_diflags(
 			di_flags |= XFS_DIFLAG_NOSYMLINKS;
 		if (xflags & XFS_XFLAG_EXTSZINHERIT)
 			di_flags |= XFS_DIFLAG_EXTSZINHERIT;
+		if (xflags & XFS_XFLAG_PROJINHERIT)
+			di_flags |= XFS_DIFLAG_PROJINHERIT;
 	} else if (S_ISREG(ip->i_d.di_mode)) {
 		if (xflags & XFS_XFLAG_REALTIME)
 			di_flags |= XFS_DIFLAG_REALTIME;
-- 
2.0.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/2] xfs: only set extent size hint when asked

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Currently the extent size hint is set unconditionally in
xfs_ioctl_setattr(), even when the FSX_EXTSIZE flag is not set. This
means we can set values from uninitialised stack variables. Hence
only set the extent size hint from userspace when both the mask
falg is set and the inode has the XFS_DIFLAG_EXTSIZE flag set to
indicate that we should have an extent size hint set on the inode.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_ioctl.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 87c3bd1..24c926b 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1231,13 +1231,25 @@ xfs_ioctl_setattr(
 
 	}
 
-	if (mask & FSX_EXTSIZE)
-		ip->i_d.di_extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
 	if (mask & FSX_XFLAGS) {
 		xfs_set_diflags(ip, fa->fsx_xflags);
 		xfs_diflags_to_linux(ip);
 	}
 
+	/*
+	 * Only set the extent size hint if we've already determined that the
+	 * extent size hint should be set on the inode. If no extent size flags
+	 * are set on the inode then unconditionally clear the extent size hint.
+	 */
+	if (mask & FSX_EXTSIZE) {
+		int	extsize = 0;
+
+		if (ip->i_d.di_flags &
+				(XFS_DIFLAG_EXTSIZE | XFS_DIFLAG_EXTSZINHERIT))
+			extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
+		ip->i_d.di_extsize = extsize;
+	}
+
 	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 
-- 
2.0.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 0/2] xfs: cleanup XFS_IOC_SETXATTR behaviour

[Iustin Pop]

On Tue, Sep 30, 2014 at 11:46:03AM +1000, Dave Chinner wrote:
> Hi folks,
> 
> A while back Iustin Pop sent a patch to fix a problem with being
> unable to set extent size hint values on directories. That patch -
> along with new xfstests functionality to always check the scratch
> device after a test - has pointed out that we allow certain
> directory only inode flags to be set on other types of inodes (e.g.
> regular files). It also pointed out that we could set extent size
> hints on inodes that don't have extent size hint flags set.

[…]

> Thoughts, comments, flames?

Thanks for sending these - I didn't forget about my patches, just was
busy with other stuff. I was planning to update those patches based on
your comments and resend them in the next week or so.

iustin

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs: only set extent size hint when asked

[Iustin Pop]

On Tue, Sep 30, 2014 at 11:46:05AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Currently the extent size hint is set unconditionally in
> xfs_ioctl_setattr(), even when the FSX_EXTSIZE flag is not set. This
> means we can set values from uninitialised stack variables. Hence
> only set the extent size hint from userspace when both the mask
> falg is set and the inode has the XFS_DIFLAG_EXTSIZE flag set to
> indicate that we should have an extent size hint set on the inode.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_ioctl.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 87c3bd1..24c926b 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1231,13 +1231,25 @@ xfs_ioctl_setattr(
>  
>  	}
>  
> -	if (mask & FSX_EXTSIZE)
> -		ip->i_d.di_extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
>  	if (mask & FSX_XFLAGS) {
>  		xfs_set_diflags(ip, fa->fsx_xflags);
>  		xfs_diflags_to_linux(ip);
>  	}
>  
> +	/*
> +	 * Only set the extent size hint if we've already determined that the
> +	 * extent size hint should be set on the inode. If no extent size flags
> +	 * are set on the inode then unconditionally clear the extent size hint.
> +	 */
> +	if (mask & FSX_EXTSIZE) {
> +		int	extsize = 0;
> +
> +		if (ip->i_d.di_flags &
> +				(XFS_DIFLAG_EXTSIZE | XFS_DIFLAG_EXTSZINHERIT))
> +			extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
> +		ip->i_d.di_extsize = extsize;

Quick question: this sounds sane, but it will have the following effect
(if I understand things correctly): updating other flags on the inode
(e.g. XFS_XFLAG_NOATIME) might change the recorded extent size. True, it
will correct the size if not appropriate and it will have a noop impact,
but still it will be an unrelated inode change. Would it make sense to
document this in the xfsctl man page then?

thanks,
iustin

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs: only set extent size hint when asked

[Dave Chinner]

On Tue, Sep 30, 2014 at 07:58:29AM +0200, Iustin Pop wrote:
> On Tue, Sep 30, 2014 at 11:46:05AM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Currently the extent size hint is set unconditionally in
> > xfs_ioctl_setattr(), even when the FSX_EXTSIZE flag is not set. This
> > means we can set values from uninitialised stack variables. Hence
> > only set the extent size hint from userspace when both the mask
> > falg is set and the inode has the XFS_DIFLAG_EXTSIZE flag set to
> > indicate that we should have an extent size hint set on the inode.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_ioctl.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 87c3bd1..24c926b 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1231,13 +1231,25 @@ xfs_ioctl_setattr(
> >  
> >  	}
> >  
> > -	if (mask & FSX_EXTSIZE)
> > -		ip->i_d.di_extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
> >  	if (mask & FSX_XFLAGS) {
> >  		xfs_set_diflags(ip, fa->fsx_xflags);
> >  		xfs_diflags_to_linux(ip);
> >  	}
> >  
> > +	/*
> > +	 * Only set the extent size hint if we've already determined that the
> > +	 * extent size hint should be set on the inode. If no extent size flags
> > +	 * are set on the inode then unconditionally clear the extent size hint.
> > +	 */
> > +	if (mask & FSX_EXTSIZE) {
> > +		int	extsize = 0;
> > +
> > +		if (ip->i_d.di_flags &
> > +				(XFS_DIFLAG_EXTSIZE | XFS_DIFLAG_EXTSZINHERIT))
> > +			extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
> > +		ip->i_d.di_extsize = extsize;
> 
> Quick question: this sounds sane, but it will have the following effect
> (if I understand things correctly): updating other flags on the inode
> (e.g. XFS_XFLAG_NOATIME) might change the recorded extent size.

That's no different to what happens before this patch. As I said in
the cover note, I'm not attempting to fix those problem with these
patches.

Besides, you're still thinking that you can just call
XFS_IOC_SETXATTR without a preceeding XFS_IOC_GETXATTR call. That's
just broken - if applications use getxattr/setxattr correctly then
this isn't an issue. i.e. do this:

	ioctl(XFS_IOC_GETXATTR, &fsx)
	fsx.fsx_xflags |= XFS_XFLAG_NOATIME;
	ioctl(XFS_IOC_SETXATTR, &fsx)

and the problem you allude to does not occur because it will set the
extent size to the same value as it currently has.

> True, it
> will correct the size if not appropriate and it will have a noop impact,
> but still it will be an unrelated inode change. Would it make sense to
> document this in the xfsctl man page then?

There's no point in documenting what *might* happen if you abuse the
interface in ways it was not intended to be used.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2] xfs: project id inheritance is a directory only flag

[Brian Foster]

On Tue, Sep 30, 2014 at 11:46:04AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> xfs_set_diflags() allows it to be set on non-directory inodes, and
> this flags errors in xfs_repair. Further, inode allocation allows
> the same directory-only flag to be inherited to non-directories.
> Make sure directory inode flags don't appear on other types of
> inodes.
> 
> This fixes several xfstests scratch fileystem corruption reports
> (e.g. xfs/050) now that xfstests checks scratch filesystems after
> test completion.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_inode.c | 4 ++--
>  fs/xfs/xfs_ioctl.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index f07b443..8ed049d 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -769,6 +769,8 @@ xfs_ialloc(
>  					di_flags |= XFS_DIFLAG_EXTSZINHERIT;
>  					ip->i_d.di_extsize = pip->i_d.di_extsize;
>  				}
> +				if (pip->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)
> +					di_flags |= XFS_DIFLAG_PROJINHERIT;
>  			} else if (S_ISREG(mode)) {
>  				if (pip->i_d.di_flags & XFS_DIFLAG_RTINHERIT)
>  					di_flags |= XFS_DIFLAG_REALTIME;
> @@ -789,8 +791,6 @@ xfs_ialloc(
>  			if ((pip->i_d.di_flags & XFS_DIFLAG_NOSYMLINKS) &&
>  			    xfs_inherit_nosymlinks)
>  				di_flags |= XFS_DIFLAG_NOSYMLINKS;
> -			if (pip->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)
> -				di_flags |= XFS_DIFLAG_PROJINHERIT;
>  			if ((pip->i_d.di_flags & XFS_DIFLAG_NODEFRAG) &&
>  			    xfs_inherit_nodefrag)
>  				di_flags |= XFS_DIFLAG_NODEFRAG;
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 7a6b406..87c3bd1 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -968,8 +968,6 @@ xfs_set_diflags(
>  		di_flags |= XFS_DIFLAG_NOATIME;
>  	if (xflags & XFS_XFLAG_NODUMP)
>  		di_flags |= XFS_DIFLAG_NODUMP;
> -	if (xflags & XFS_XFLAG_PROJINHERIT)
> -		di_flags |= XFS_DIFLAG_PROJINHERIT;
>  	if (xflags & XFS_XFLAG_NODEFRAG)
>  		di_flags |= XFS_DIFLAG_NODEFRAG;
>  	if (xflags & XFS_XFLAG_FILESTREAM)
> @@ -981,6 +979,8 @@ xfs_set_diflags(
>  			di_flags |= XFS_DIFLAG_NOSYMLINKS;
>  		if (xflags & XFS_XFLAG_EXTSZINHERIT)
>  			di_flags |= XFS_DIFLAG_EXTSZINHERIT;
> +		if (xflags & XFS_XFLAG_PROJINHERIT)
> +			di_flags |= XFS_DIFLAG_PROJINHERIT;
>  	} else if (S_ISREG(ip->i_d.di_mode)) {
>  		if (xflags & XFS_XFLAG_REALTIME)
>  			di_flags |= XFS_DIFLAG_REALTIME;
> -- 
> 2.0.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs: only set extent size hint when asked

[Brian Foster]

On Tue, Sep 30, 2014 at 11:46:05AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Currently the extent size hint is set unconditionally in
> xfs_ioctl_setattr(), even when the FSX_EXTSIZE flag is not set. This
> means we can set values from uninitialised stack variables. Hence
> only set the extent size hint from userspace when both the mask
> falg is set and the inode has the XFS_DIFLAG_EXTSIZE flag set to
> indicate that we should have an extent size hint set on the inode.
> 

I'm not sure what you mean here by FSX_EXTSIZE not being checked. It
looks like FSX_EXTSIZE is checked before and after the patch.

Regardless, the fix looks Ok to me...

Reviewed-by: Brian Foster <bfoster@redhat.com>

> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_ioctl.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 87c3bd1..24c926b 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1231,13 +1231,25 @@ xfs_ioctl_setattr(
>  
>  	}
>  
> -	if (mask & FSX_EXTSIZE)
> -		ip->i_d.di_extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
>  	if (mask & FSX_XFLAGS) {
>  		xfs_set_diflags(ip, fa->fsx_xflags);
>  		xfs_diflags_to_linux(ip);
>  	}
>  
> +	/*
> +	 * Only set the extent size hint if we've already determined that the
> +	 * extent size hint should be set on the inode. If no extent size flags
> +	 * are set on the inode then unconditionally clear the extent size hint.
> +	 */
> +	if (mask & FSX_EXTSIZE) {
> +		int	extsize = 0;
> +
> +		if (ip->i_d.di_flags &
> +				(XFS_DIFLAG_EXTSIZE | XFS_DIFLAG_EXTSZINHERIT))
> +			extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
> +		ip->i_d.di_extsize = extsize;
> +	}
> +
>  	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  
> -- 
> 2.0.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs: only set extent size hint when asked

[Dave Chinner]

On Wed, Oct 01, 2014 at 07:59:59AM -0400, Brian Foster wrote:
> On Tue, Sep 30, 2014 at 11:46:05AM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Currently the extent size hint is set unconditionally in
> > xfs_ioctl_setattr(), even when the FSX_EXTSIZE flag is not set. This
> > means we can set values from uninitialised stack variables. Hence
> > only set the extent size hint from userspace when both the mask
> > falg is set and the inode has the XFS_DIFLAG_EXTSIZE flag set to
> > indicate that we should have an extent size hint set on the inode.
> > 
> 
> I'm not sure what you mean here by FSX_EXTSIZE not being checked. It
> looks like FSX_EXTSIZE is checked before and after the patch.

Yup, I wrote the commit message, then did the fix and forgot to
update the message after I realised it was wrong. I've fixed the
commit message.

> Regardless, the fix looks Ok to me...
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Thanks!

-Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs