| Message ID | 1470120540-15135-1-git-send-email-zlang@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Aug 02, 2016 at 02:49:00PM +0800, Zorro Lang wrote: > After XFS_GETNEXTQUOTA feature has been merged into linux kernel and > xfsprogs, xfs_quota use Q_XGETNEXTQUOTA for report and dump, and > fall back to old XFS_GETQUOTA ioctl if XFS_GETNEXTQUOTA fails. > > But when XFS_GETNEXTQUOTA fails, xfs_quota print a warning as > "XFS_GETQUOTA: Invalid argument". That's due to kernel can't > recognize XFS_GETNEXTQUOTA ioctl and return EINVAL. At this time, > the warning is helpless, xfs_quota just need to fall back. We'd still want to report other errors, right?
On Tue, Aug 02, 2016 at 05:27:21AM -0700, Christoph Hellwig wrote: > On Tue, Aug 02, 2016 at 02:49:00PM +0800, Zorro Lang wrote: > > After XFS_GETNEXTQUOTA feature has been merged into linux kernel and > > xfsprogs, xfs_quota use Q_XGETNEXTQUOTA for report and dump, and > > fall back to old XFS_GETQUOTA ioctl if XFS_GETNEXTQUOTA fails. > > > > But when XFS_GETNEXTQUOTA fails, xfs_quota print a warning as > > "XFS_GETQUOTA: Invalid argument". That's due to kernel can't > > recognize XFS_GETNEXTQUOTA ioctl and return EINVAL. At this time, > > the warning is helpless, xfs_quota just need to fall back. > > We'd still want to report other errors, right? Yes. This patch will make xfs_quota's report and dump command report nothing if XFS_GETNEXTQUOTA fails and falls back to XFS_GETQUOTA. But if XFS_GETQUOTA fails, it'll report errors. As I mentioned in email, we don't report errors if XFS_GETNEXTQUOTA fails, or we don't report errors if kernel has no XFS_GETNEXTQUOTA feature? The first one won't report any errors from XFS_GETNEXTQUOTA call, include kernel has no this feature. So: "cmd == XFS_GETQUOTA" or "!(cmd == XFS_GETNEXTQUOTA && errno == EINVAL)" I think they all make sense. Do you have any suggestions? Thanks, Zorro > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs
On 8/2/16 7:27 AM, Christoph Hellwig wrote: > On Tue, Aug 02, 2016 at 02:49:00PM +0800, Zorro Lang wrote: >> After XFS_GETNEXTQUOTA feature has been merged into linux kernel and >> xfsprogs, xfs_quota use Q_XGETNEXTQUOTA for report and dump, and >> fall back to old XFS_GETQUOTA ioctl if XFS_GETNEXTQUOTA fails. >> >> But when XFS_GETNEXTQUOTA fails, xfs_quota print a warning as >> "XFS_GETQUOTA: Invalid argument". That's due to kernel can't >> recognize XFS_GETNEXTQUOTA ioctl and return EINVAL. At this time, >> the warning is helpless, xfs_quota just need to fall back. > > We'd still want to report other errors, right? I advised Zorro to do it this way, because -EINVAL can have a few meanings, and we don't know for sure why we got it. Could be a bad cmd, or a bad type, or ... If it fails, we're going to fall back anyway, so for any other error we'd print it twice; on the fallback, we'd print the real unexpected error anyway, so I think the user will get the relevant information in this case. But I guess we could do: + /* EINVAL is expected for XFS_GETNEXTQUOTA on older kernels */ if (xfsquotactl(cmd, dev, type, id, (void *)&d) < 0) { + if (errno != ENOENT && errno != ENOSYS && errno != ESRCH && + (cmd == XFS_GETNEXTQUOTA && errno != EINVAL) and then change what we print (not perror("XFS_GETQUOTA") in all cases)? But if we got i.e. EPERM, we'd print the EPERM error twice; once for the first call, and once for the fallback. I suppose that'd be ok, but not sure it's helpful. -Eric
On 8/2/16 8:14 AM, Zorro Lang wrote: > On Tue, Aug 02, 2016 at 05:27:21AM -0700, Christoph Hellwig wrote: >> On Tue, Aug 02, 2016 at 02:49:00PM +0800, Zorro Lang wrote: >>> After XFS_GETNEXTQUOTA feature has been merged into linux kernel and >>> xfsprogs, xfs_quota use Q_XGETNEXTQUOTA for report and dump, and >>> fall back to old XFS_GETQUOTA ioctl if XFS_GETNEXTQUOTA fails. >>> >>> But when XFS_GETNEXTQUOTA fails, xfs_quota print a warning as >>> "XFS_GETQUOTA: Invalid argument". That's due to kernel can't >>> recognize XFS_GETNEXTQUOTA ioctl and return EINVAL. At this time, >>> the warning is helpless, xfs_quota just need to fall back. >> >> We'd still want to report other errors, right? > > Yes. This patch will make xfs_quota's report and dump command report > nothing if XFS_GETNEXTQUOTA fails and falls back to XFS_GETQUOTA. > > But if XFS_GETQUOTA fails, it'll report errors. > > As I mentioned in email, we don't report errors if XFS_GETNEXTQUOTA > fails, or we don't report errors if kernel has no XFS_GETNEXTQUOTA > feature? The first one won't report any errors from XFS_GETNEXTQUOTA call, > include kernel has no this feature. > > So: > "cmd == XFS_GETQUOTA" or "!(cmd == XFS_GETNEXTQUOTA && errno == EINVAL)" Oh, I see, this is what I was trying to do before coffee in my earlier reply, and failed. :) > I think they all make sense. Do you have any suggestions? Ignoring EINVAL only for XFS_GETNEXTQUOTA seems like a reasonable idea - we might print two warnings for other errors, though - that might be a little odd, but not terrible. I think the patch as it stands is ok; unexpected errors will be caught and printed on the fallback, and we don't need extra complexity around printing two different command names that way. But if there's preference for printing failure information for both calls, that's fine with me, as long as we filter out EINVAL for GETNEXTQUOTA. -Eric > Thanks, > Zorro > >> >> _______________________________________________ >> 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 >
diff --git a/quota/report.c b/quota/report.c index 59290e1..70220b4 100644 --- a/quota/report.c +++ b/quota/report.c @@ -90,8 +90,10 @@ dump_file( else cmd = XFS_GETQUOTA; + /* Fall back silently if XFS_GETNEXTQUOTA fails, warn on XFS_GETQUOTA */ if (xfsquotactl(cmd, dev, type, id, (void *)&d) < 0) { - if (errno != ENOENT && errno != ENOSYS && errno != ESRCH) + if (errno != ENOENT && errno != ENOSYS && errno != ESRCH && + cmd == XFS_GETQUOTA) perror("XFS_GETQUOTA"); return 0; } @@ -347,8 +349,10 @@ report_mount( else cmd = XFS_GETQUOTA; + /* Fall back silently if XFS_GETNEXTQUOTA fails, warn on XFS_GETQUOTA*/ if (xfsquotactl(cmd, dev, type, id, (void *)&d) < 0) { - if (errno != ENOENT && errno != ENOSYS && errno != ESRCH) + if (errno != ENOENT && errno != ENOSYS && errno != ESRCH && + cmd == XFS_GETQUOTA) perror("XFS_GETQUOTA"); return 0; }
After XFS_GETNEXTQUOTA feature has been merged into linux kernel and xfsprogs, xfs_quota use Q_XGETNEXTQUOTA for report and dump, and fall back to old XFS_GETQUOTA ioctl if XFS_GETNEXTQUOTA fails. But when XFS_GETNEXTQUOTA fails, xfs_quota print a warning as "XFS_GETQUOTA: Invalid argument". That's due to kernel can't recognize XFS_GETNEXTQUOTA ioctl and return EINVAL. At this time, the warning is helpless, xfs_quota just need to fall back. Signed-off-by: Zorro Lang <zlang@redhat.com> --- Hi, Both dump_file and report_mount have this problem, so I fix them together. xfstests xfs/299 can reproduce this bug in report_mount, the newest xfs/106 can reproduce both(dump and report, hope I didn't miss others:). This patch checks "if cmd == XFS_GETQUOTA", but I'm thinking about if we should check "if !(cmd == XFS_GETNEXTQUOTA && errno == EINVAL)"? The first one don't print all errors from XFS_GETNEXTQUOTA, but the second one only for EINVAL error. So the question become should we: 1) fall back silently if XFS_GETNEXTQUOTA fails? 2) Or fall back silently if kernel has no XFS_GETNEXTQUOTA feature? I think both of them make sense. What do you think? Thanks, Zorro quota/report.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)