xfs_quota: fall back silently if XFS_GETNEXTQUOTA fails
diff mbox

Message ID 1470120540-15135-1-git-send-email-zlang@redhat.com
State Accepted
Headers show

Commit Message

Zorro Lang Aug. 2, 2016, 6:49 a.m. UTC
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(-)

Comments

Christoph Hellwig Aug. 2, 2016, 12:27 p.m. UTC | #1
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?
Zorro Lang Aug. 2, 2016, 1:14 p.m. UTC | #2
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
Eric Sandeen Aug. 2, 2016, 3:06 p.m. UTC | #3
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
Eric Sandeen Aug. 2, 2016, 3:54 p.m. UTC | #4
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
>

Patch
diff mbox

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