[PATCH] xfs_quota: XFS_XFLAG_PROJINHERIT is only for dirs

[PATCH] xfs_quota: XFS_XFLAG_PROJINHERIT is only for dirs

[Eric Sandeen]

xfs_quota has long set XFS_XFLAG_PROJINHERIT on all files,
and tested for presence on all files.  However, Dave's semi-recent
xfs_repair update is now flagging this as an error:

bd5683f xfs_repair: validate inode di_flags field

       if (flags & (XFS_DIFLAG_RTINHERIT |
                    XFS_DIFLAG_EXTSZINHERIT |
                    XFS_DIFLAG_PROJINHERIT |
                    XFS_DIFLAG_NOSYMLINKS)) {
               /* must be a directory */
               if (di_mode && !S_ISDIR(di_mode)) {
                       do_warn(_(
               "directory flags set on non-directory inode %llu"),
                               lino);

In tests 050, 107, 134, 244, 261 and 287, inserting a _check_scratch_fs
will cause them to fail due to repair finding these errors.

On the one hand, quota has set this flag everywhere for many years.  On
the other hand, an *INHERIT flag makes no sense on a non-dir.  So I think
the right way to go is to tell quota to stop setting (and checking for)
the flag unconditionally, and do it only for S_ISDIR.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

note: A little unsure of the new check in check_project.  Should we just
leave that to repair?

diff --git a/quota/project.c b/quota/project.c
index a2c7046..7042015 100644
--- a/quota/project.c
+++ b/quota/project.c
@@ -126,8 +126,11 @@ check_project(
 			printf(_("%s - project identifier is not set"
 				 " (inode=%u, tree=%u)\n"),
 				path, fsx.fsx_projid, (unsigned int)prid);
-		if (!(fsx.fsx_xflags & XFS_XFLAG_PROJINHERIT))
-			printf(_("%s - project inheritance flag is not set\n"),
+		if (S_ISDIR(stat->st_mode) && !(fsx.fsx_xflags & XFS_XFLAG_PROJINHERIT))
+			printf(_("%s - project inheritance flag is not set on dir\n"),
+				path);
+		if (!S_ISDIR(stat->st_mode) && (fsx.fsx_xflags & XFS_XFLAG_PROJINHERIT))
+			printf(_("%s - project inheritance flag is set on non-dir\n"),
 				path);
 	}
 	if (fd != -1)
@@ -219,7 +222,8 @@ setup_project(
 	}
 
 	fsx.fsx_projid = prid;
-	fsx.fsx_xflags |= XFS_XFLAG_PROJINHERIT;
+	if (S_ISDIR(stat->st_mode))
+		fsx.fsx_xflags |= XFS_XFLAG_PROJINHERIT;
 	if (xfsctl(path, fd, XFS_IOC_FSSETXATTR, &fsx) < 0) {
 		exitcode = 1;
 		fprintf(stderr, _("%s: cannot set project on %s: %s\n"),

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

Re: [PATCH] xfs_quota: XFS_XFLAG_PROJINHERIT is only for dirs

[Christoph Hellwig]

On Fri, Oct 12, 2012 at 11:19:55AM -0500, Eric Sandeen wrote:
> xfs_quota has long set XFS_XFLAG_PROJINHERIT on all files,
> and tested for presence on all files.  However, Dave's semi-recent
> xfs_repair update is now flagging this as an error:

I think we should rever that part of the repair patch.  While there
really is not point to have XFS_XFLAG_PROJINHERIT set on non-directory
files we have been setting it for year, so repair should cope with that.

I'd also like to have a test checking that repair does not complain
about XFS_XFLAG_PROJINHERIT being set on a non-directory to verify this.

For the future I'm fine with not setting it on non-directories anymore,
so:

Reviewed-by: Christoph Hellwig <hch@lst.de>

for this patch after a slight update to the commit message.

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

Re: [PATCH] xfs_quota: XFS_XFLAG_PROJINHERIT is only for dirs

[Eric Sandeen]

On 10/13/12 10:52 AM, Christoph Hellwig wrote:
> On Fri, Oct 12, 2012 at 11:19:55AM -0500, Eric Sandeen wrote:
>> xfs_quota has long set XFS_XFLAG_PROJINHERIT on all files,
>> and tested for presence on all files.  However, Dave's semi-recent
>> xfs_repair update is now flagging this as an error:
> 
> I think we should rever that part of the repair patch.  While there
> really is not point to have XFS_XFLAG_PROJINHERIT set on non-directory
> files we have been setting it for year, so repair should cope with that.
> 
> I'd also like to have a test checking that repair does not complain
> about XFS_XFLAG_PROJINHERIT being set on a non-directory to verify this.

Ok, seems reasonable.  Presumably I'd need to remove my change
to check_project() as well.  I should probably send V2, then.

Kind of amazing that this was missed for so long ;)

Thanks,
-Eric

> For the future I'm fine with not setting it on non-directories anymore,
> so:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> for this patch after a slight update to the commit message.
> 
> _______________________________________________
> 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] xfs_quota: XFS_XFLAG_PROJINHERIT is only for dirs

[Dave Chinner]

On Sat, Oct 13, 2012 at 11:52:05AM -0400, Christoph Hellwig wrote:
> On Fri, Oct 12, 2012 at 11:19:55AM -0500, Eric Sandeen wrote:
> > xfs_quota has long set XFS_XFLAG_PROJINHERIT on all files,
> > and tested for presence on all files.  However, Dave's semi-recent
> > xfs_repair update is now flagging this as an error:
> 
> I think we should rever that part of the repair patch.  While there
> really is not point to have XFS_XFLAG_PROJINHERIT set on non-directory
> files we have been setting it for year, so repair should cope with that.

Repair does cope with it - it issues a warning and clears the flag.
It doesn't stop, it simply fixes an inconsistency in the inode flags.

The main problem, by the sounds of it, is that repair issues a
warning that it is clearing the flags that should not be set. This
is what makes check_scratch_fs fail because of the extra output.
That's easy to fix - filter the line from the repair output and be
done with it. In future (with Eric's patch) this situation won't
occur.

So, really, I think the only thing that needs modifying to handle
this situation is a filter update to xfstests...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfs_quota: XFS_XFLAG_PROJINHERIT is only for dirs

[Eric Sandeen]

On 10/13/12 6:34 PM, Dave Chinner wrote:
> On Sat, Oct 13, 2012 at 11:52:05AM -0400, Christoph Hellwig wrote:
>> On Fri, Oct 12, 2012 at 11:19:55AM -0500, Eric Sandeen wrote:
>>> xfs_quota has long set XFS_XFLAG_PROJINHERIT on all files,
>>> and tested for presence on all files.  However, Dave's semi-recent
>>> xfs_repair update is now flagging this as an error:
>>
>> I think we should rever that part of the repair patch.  While there
>> really is not point to have XFS_XFLAG_PROJINHERIT set on non-directory
>> files we have been setting it for year, so repair should cope with that.
> 
> Repair does cope with it - it issues a warning and clears the flag.
> It doesn't stop, it simply fixes an inconsistency in the inode flags.
> 
> The main problem, by the sounds of it, is that repair issues a
> warning that it is clearing the flags that should not be set. This
> is what makes check_scratch_fs fail because of the extra output.
> That's easy to fix - filter the line from the repair output and be
> done with it. In future (with Eric's patch) this situation won't
> occur.
> 
> So, really, I think the only thing that needs modifying to handle
> this situation is a filter update to xfstests...

I must be missing something; quota will continue to set it and repair
will continue to clear it.  One should probably match the other right?
So one or the other should change.

-Eric

> Cheers,
> 
> Dave.
> 

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

Re: [PATCH] xfs_quota: XFS_XFLAG_PROJINHERIT is only for dirs

[Dave Chinner]

On Sat, Oct 13, 2012 at 07:05:51PM -0500, Eric Sandeen wrote:
> On 10/13/12 6:34 PM, Dave Chinner wrote:
> > On Sat, Oct 13, 2012 at 11:52:05AM -0400, Christoph Hellwig wrote:
> >> On Fri, Oct 12, 2012 at 11:19:55AM -0500, Eric Sandeen wrote:
> >>> xfs_quota has long set XFS_XFLAG_PROJINHERIT on all files,
> >>> and tested for presence on all files.  However, Dave's semi-recent
> >>> xfs_repair update is now flagging this as an error:
> >>
> >> I think we should rever that part of the repair patch.  While there
> >> really is not point to have XFS_XFLAG_PROJINHERIT set on non-directory
> >> files we have been setting it for year, so repair should cope with that.
> > 
> > Repair does cope with it - it issues a warning and clears the flag.
> > It doesn't stop, it simply fixes an inconsistency in the inode flags.
> > 
> > The main problem, by the sounds of it, is that repair issues a
> > warning that it is clearing the flags that should not be set. This
> > is what makes check_scratch_fs fail because of the extra output.
> > That's easy to fix - filter the line from the repair output and be
> > done with it. In future (with Eric's patch) this situation won't
> > occur.
> > 
> > So, really, I think the only thing that needs modifying to handle
> > this situation is a filter update to xfstests...
> 
> I must be missing something; quota will continue to set it and repair
> will continue to clear it.  One should probably match the other right?
> So one or the other should change.

Sorry, I wasn't particularly clear - if your patch to quota goes in,
the problem goes away in future and we should simply filter the
warning in xfstests to handle the present issue....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfs_quota: XFS_XFLAG_PROJINHERIT is only for dirs

[Ben Myers]

Hey Eric,

On Sun, Oct 14, 2012 at 11:14:12AM +1100, Dave Chinner wrote:
> On Sat, Oct 13, 2012 at 07:05:51PM -0500, Eric Sandeen wrote:
> > On 10/13/12 6:34 PM, Dave Chinner wrote:
> > > On Sat, Oct 13, 2012 at 11:52:05AM -0400, Christoph Hellwig wrote:
> > >> On Fri, Oct 12, 2012 at 11:19:55AM -0500, Eric Sandeen wrote:
> > >>> xfs_quota has long set XFS_XFLAG_PROJINHERIT on all files,
> > >>> and tested for presence on all files.  However, Dave's semi-recent
> > >>> xfs_repair update is now flagging this as an error:
> > >>
> > >> I think we should rever that part of the repair patch.  While there
> > >> really is not point to have XFS_XFLAG_PROJINHERIT set on non-directory
> > >> files we have been setting it for year, so repair should cope with that.
> > > 
> > > Repair does cope with it - it issues a warning and clears the flag.
> > > It doesn't stop, it simply fixes an inconsistency in the inode flags.
> > > 
> > > The main problem, by the sounds of it, is that repair issues a
> > > warning that it is clearing the flags that should not be set. This
> > > is what makes check_scratch_fs fail because of the extra output.
> > > That's easy to fix - filter the line from the repair output and be
> > > done with it. In future (with Eric's patch) this situation won't
> > > occur.
> > > 
> > > So, really, I think the only thing that needs modifying to handle
> > > this situation is a filter update to xfstests...
> > 
> > I must be missing something; quota will continue to set it and repair
> > will continue to clear it.  One should probably match the other right?
> > So one or the other should change.
> 
> Sorry, I wasn't particularly clear - if your patch to quota goes in,
> the problem goes away in future and we should simply filter the
> warning in xfstests to handle the present issue....

I think Dave has an interesting idea here. 

You already have:

1) only set XFS_XFLAG_PROJINHERIT on directories in setup_project,

2) update check_project to print the right warning based upon the above,

Now all you need is:

3) update _check_scratch_fs to filter
	"directory flags set on non-directory inode %llu"

I guess the downside of that is the test might subsequently miss other
related failure modes of xfs_repair.  Maybe it would be better to make
xfs_repair have a separate error message for this specific case, and
then filter that out of the test output.  How would you know when it's
ok to remove the filter?

Regards,
Ben

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

Re: [PATCH] xfs_quota: XFS_XFLAG_PROJINHERIT is only for dirs

[Dave Chinner]

On Tue, Oct 16, 2012 at 03:17:52PM -0500, Ben Myers wrote:
> Hey Eric,
> 
> On Sun, Oct 14, 2012 at 11:14:12AM +1100, Dave Chinner wrote:
> > On Sat, Oct 13, 2012 at 07:05:51PM -0500, Eric Sandeen wrote:
> > > On 10/13/12 6:34 PM, Dave Chinner wrote:
> > > > On Sat, Oct 13, 2012 at 11:52:05AM -0400, Christoph Hellwig wrote:
> > > >> On Fri, Oct 12, 2012 at 11:19:55AM -0500, Eric Sandeen wrote:
> > > >>> xfs_quota has long set XFS_XFLAG_PROJINHERIT on all files,
> > > >>> and tested for presence on all files.  However, Dave's semi-recent
> > > >>> xfs_repair update is now flagging this as an error:
> > > >>
> > > >> I think we should rever that part of the repair patch.  While there
> > > >> really is not point to have XFS_XFLAG_PROJINHERIT set on non-directory
> > > >> files we have been setting it for year, so repair should cope with that.
> > > > 
> > > > Repair does cope with it - it issues a warning and clears the flag.
> > > > It doesn't stop, it simply fixes an inconsistency in the inode flags.
> > > > 
> > > > The main problem, by the sounds of it, is that repair issues a
> > > > warning that it is clearing the flags that should not be set. This
> > > > is what makes check_scratch_fs fail because of the extra output.
> > > > That's easy to fix - filter the line from the repair output and be
> > > > done with it. In future (with Eric's patch) this situation won't
> > > > occur.
> > > > 
> > > > So, really, I think the only thing that needs modifying to handle
> > > > this situation is a filter update to xfstests...
> > > 
> > > I must be missing something; quota will continue to set it and repair
> > > will continue to clear it.  One should probably match the other right?
> > > So one or the other should change.
> > 
> > Sorry, I wasn't particularly clear - if your patch to quota goes in,
> > the problem goes away in future and we should simply filter the
> > warning in xfstests to handle the present issue....
> 
> I think Dave has an interesting idea here. 
> 
> You already have:
> 
> 1) only set XFS_XFLAG_PROJINHERIT on directories in setup_project,
> 
> 2) update check_project to print the right warning based upon the above,
> 
> Now all you need is:
> 
> 3) update _check_scratch_fs to filter
> 	"directory flags set on non-directory inode %llu"
> 
> I guess the downside of that is the test might subsequently miss other
> related failure modes of xfs_repair.  Maybe it would be better to make
> xfs_repair have a separate error message for this specific case, and
> then filter that out of the test output.  How would you know when it's
> ok to remove the filter?

I don't think it ever would be removed, because xfstests is used for
testing older versions of XFS as well as TOT.

As it is, I don't think that the warning from repair is really
sufficient to warrant a test failure. We've never really cared about
this problem, it certainly isn't fatal to the filesystem, and the
kernel ensures that directory specific flags only affect directory
inodes so we aren't getting strange behaviour because of it. So I
don't really see any problem with just filtering it out
unconditionally from repair in xfstests.

If we really want to ensure that we don't have directory flags set
on a regular file in future, we should write a test that
specifically tries to do this and run repair with a custom filter on
the filesystem.  i.e.  make sure the kernel rejects directory only
flags being set on non-directories, followed by adding them via
xfs_db and ensuring that xfs_repair warns about them.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfs_quota: XFS_XFLAG_PROJINHERIT is only for dirs

[Eric Sandeen]

On 10/16/12 3:17 PM, Ben Myers wrote:
> Hey Eric,
> 
> On Sun, Oct 14, 2012 at 11:14:12AM +1100, Dave Chinner wrote:
>> On Sat, Oct 13, 2012 at 07:05:51PM -0500, Eric Sandeen wrote:
>>> On 10/13/12 6:34 PM, Dave Chinner wrote:
>>>> On Sat, Oct 13, 2012 at 11:52:05AM -0400, Christoph Hellwig wrote:
>>>>> On Fri, Oct 12, 2012 at 11:19:55AM -0500, Eric Sandeen wrote:
>>>>>> xfs_quota has long set XFS_XFLAG_PROJINHERIT on all files,
>>>>>> and tested for presence on all files.  However, Dave's semi-recent
>>>>>> xfs_repair update is now flagging this as an error:
>>>>>
>>>>> I think we should rever that part of the repair patch.  While there
>>>>> really is not point to have XFS_XFLAG_PROJINHERIT set on non-directory
>>>>> files we have been setting it for year, so repair should cope with that.
>>>>
>>>> Repair does cope with it - it issues a warning and clears the flag.
>>>> It doesn't stop, it simply fixes an inconsistency in the inode flags.
>>>>
>>>> The main problem, by the sounds of it, is that repair issues a
>>>> warning that it is clearing the flags that should not be set. This
>>>> is what makes check_scratch_fs fail because of the extra output.
>>>> That's easy to fix - filter the line from the repair output and be
>>>> done with it. In future (with Eric's patch) this situation won't
>>>> occur.
>>>>
>>>> So, really, I think the only thing that needs modifying to handle
>>>> this situation is a filter update to xfstests...
>>>
>>> I must be missing something; quota will continue to set it and repair
>>> will continue to clear it.  One should probably match the other right?
>>> So one or the other should change.
>>
>> Sorry, I wasn't particularly clear - if your patch to quota goes in,
>> the problem goes away in future and we should simply filter the
>> warning in xfstests to handle the present issue....
> 
> I think Dave has an interesting idea here. 
> 
> You already have:
> 
> 1) only set XFS_XFLAG_PROJINHERIT on directories in setup_project,
> 
> 2) update check_project to print the right warning based upon the above,
> 
> Now all you need is:
> 
> 3) update _check_scratch_fs to filter
> 	"directory flags set on non-directory inode %llu"

Well, there is no project ID test which runs _check_scratch_fs anyway.

So at least for today, it's not an issue.

-Eric

> I guess the downside of that is the test might subsequently miss other
> related failure modes of xfs_repair.  Maybe it would be better to make
> xfs_repair have a separate error message for this specific case, and
> then filter that out of the test output.  How would you know when it's
> ok to remove the filter?
> 
> Regards,
> Ben
> 

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

Re: [PATCH] xfs_quota: XFS_XFLAG_PROJINHERIT is only for dirs

[Christoph Hellwig]

On Sun, Oct 14, 2012 at 11:14:12AM +1100, Dave Chinner wrote:
> Sorry, I wasn't particularly clear - if your patch to quota goes in,
> the problem goes away in future and we should simply filter the
> warning in xfstests to handle the present issue....

I'd suggest to stil clear it but not warn.  It's something that is a)
harmless and b) could be set in normal operation for a long time.  Just
clear it silently and add a comment.

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