xfs_quota: XFS_XFLAG_PROJINHERIT is only for dirs

PATCH In review Eric Sandeen 5 participants 10 messages Oct 12, 2012 – Oct 23, 2012

Share & tools

Eric Sandeen

author

2012-10-12 16:18 UTC #0 lore raw

PATCH 1/1 xfs_quota: XFS_XFLAG_PROJINHERIT is only for dirs

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 <[email protected]>

---

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

quota/project.c +7 -3

index a2c7046..7042015 100644
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

Reviewed-by on v1 1/1

2012-10-13 15:50 UTC #1 lore raw

Quoted 3 lines · 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:

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 <[email protected]>

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

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

2012-10-13 21:20 UTC #2 lore raw

Quoted 11 lines · Christoph Hellwig

Quoted 3 lines · 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:
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

Quoted 12 lines

For the future I'm fine with not setting it on non-directories anymore, so:
Reviewed-by Christoph Hellwig <[email protected]>
for this patch after a slight update to the commit message.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2012-10-13 23:33 UTC #3 lore raw

In reply to Christoph Hellwig

Quoted 8 lines · Christoph Hellwig

Quoted 3 lines · 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:
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.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

author

2012-10-14 00:04 UTC #4 lore raw

Quoted 20 lines · Dave Chinner

Quoted 8 lines · Christoph Hellwig
Quoted 3 lines · 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:
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.

Quoted 2 lines

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

Quoted 4 lines

Cheers,
Dave.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2012-10-14 00:12 UTC #5 lore raw

Quoted 21 lines · Eric Sandeen

Quoted 20 lines · Dave Chinner
Quoted 8 lines · Christoph Hellwig
Quoted 3 lines · 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:
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.

Quoted 6 lines

Quoted 2 lines
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.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Ben Myers

2012-10-16 20:16 UTC #6 lore raw

Hey Eric,

Quoted 25 lines · Dave Chinner

Quoted 24 lines · Eric Sandeen
Quoted 22 lines · Dave Chinner
Quoted 8 lines · Christoph Hellwig
Quoted 3 lines · 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:
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...

Quoted 7 lines

Quoted 3 lines
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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2012-10-16 20:32 UTC #7 lore raw

Quoted 46 lines · Ben Myers

Hey Eric,
Quoted 25 lines · Dave Chinner
Quoted 24 lines · Eric Sandeen
Quoted 22 lines · Dave Chinner
Quoted 8 lines · Christoph Hellwig
Quoted 3 lines · 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:
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...
Quoted 7 lines
Quoted 3 lines
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:

Quoted 8 lines

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.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

author

2012-10-17 15:53 UTC #8 lore raw

In reply to Ben Myers

Quoted 40 lines · Ben Myers

Hey Eric,
Quoted 25 lines · Dave Chinner
Quoted 24 lines · Eric Sandeen
Quoted 22 lines · Dave Chinner
Quoted 8 lines · Christoph Hellwig
Quoted 3 lines · 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:
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...
Quoted 7 lines
Quoted 3 lines
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:

Quoted 8 lines

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

Quoted 9 lines

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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

2012-10-23 12:33 UTC #9 lore raw

In reply to Dave Chinner

Quoted 3 lines · Dave Chinner

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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs