Bug 234413

This issue is visible when samba operates on a zfs file system but I think it has issued more problems.

When a call to quotactl() is made on a zfs dataset, the error generated under 11.2 FreeBSD is: Operation not supported (ENOTSUP). Which is logical because quotactl() is only supported by the ufs file system.

Under 12.0-RELEASE, you get: No such file or directory (ENOENT).

As a result, with samba, you can see some error messages claiming "No such file or directory" each time you create or delete a file on a samba share.

Created attachment 200570 [details]
C code to test the error returned by quotactl()

Reading the man page of quotactl(), I came to the conclusion that it shouldn't return ENOENT nor ENOTSUP (EOPNOTSUPP) but rather EINVAL error in the case the file system isn't ufs. ENOTSUP seems reserved when the kernel has no support for quota. And kern.features.ufs_quota is enabled both in 11.2 and 12.0 generic kernels.

[EINVAL]		The cmd	argument or the	command	type is	invalid.  In
			Q_GETQUOTASIZE,	Q_GETQUOTA, Q_SETQUOTA,	and Q_SETUSE,
			quotas are not currently enabled for this file system.

Comment 4 Mark Johnston 2019-01-09 06:51:42 UTC

ZFS implements quotactl(2) as of r336017; the man page needs to be updated.

Which quota operation is failing in your case?

(In reply to Mark Johnston from comment #4)
I wrote it in my first post (sorry for the double post by the way): it's Samba which don't handle the error returned by 12.0-RELEASE (ENOENT) and flood tty0 with error messages. There is at least two workarounds to avoid these errors.

Details here: https://forums.freebsd.org/threads/samba-quota-errors-logged.68832/

My point is that I don't think it's normal that quotactl() returns ENOENT even if the quota system is now implemented on zfs. I and many people don't use quota. So, in this case, what should be the correct error? Samba handles ENOTSUP and EINVAL errors, not ENOENT.

If ENOENT is definitively the suitable error for this case (zfs with no quota enabled), then the Samba code must be patched, for FreeBSD at least.

Comment 6 Mark Johnston 2019-01-09 18:55:00 UTC

(In reply to Emrion from comment #5)
I was wondering specifically which quotactl() operation was returning ENOENT.  It's a bit more clear now; we have the following in zfs_getquota():

 146         if (quotaobj == 0 || zfsvfs->z_replay) {                                                                                                         
 147                 error = ENOENT;                                                                                                                          
 148                 goto done;                                                                                                                               
 149         }

whereas UFS does:

1302         dqvp = ump->um_quotas[type];                                                                                                                     
1303         if (dqvp == NULLVP || (ump->um_qflags[type] & QTF_CLOSING)) {                                                                                    
1304                 *dqp = NODQUOT;                                                                                                                          
1305                 UFS_UNLOCK(ump);                                                                                                                         
1306                 return (EINVAL);                                                                                                                         
1307         }

This use of EINVAL isn't documented in the man page.  Linux's quotactl() doesn't document it either, but I presume that it always returns EINVAL in this case.

(In reply to Mark Johnston from comment #6)
Sorry, I didn't understand your question. Now, you've seen the concerned snippet code of Samba or the code I posted here for testing purpose. It's actually Q_GETQUOTA.

Thanks for providing the relevant portion of code inside FreeBSD. I found it comes from zfs_vfsops.c. I can just suppose that zfsvfs->z_groupquota_obj and zfsvfs->z_userquota_are null when there is no defined quota for the current user / group. Then, NOENT is returned. Is this a bug?

Comment 8 Sean Eric Fagan 2019-01-09 21:17:07 UTC

(In reply to Emrion from comment #7)
That's a good question.  I picked ENOENT because of similarity to other code, but going with UFS compatibility is a stronger argument.

The main intended use for zfs quota support was rpc.quotad, and it doesn't care about the errno.

Samba logs an error according to

  if (errno != ENOTSUP && errno != EINVAL) 

So, to avoid excessive logging, it would be nice to replace the error code with either ENOTSUP or EINVAL. IMHO it makes more sense to use ENOTSUP but this code is not documented in quotactl(2) at all. For UFS compatibility, we should use EINVAL but according to man page, this error code is not applicable. 

I suggest to use ENOTSUP if there is no zfs quota.

Comment 10 Mark Johnston 2019-01-10 16:40:50 UTC

(In reply to topical from comment #9)
I don't think it makes sense to return EOPNOTSUPP when Q_GETQUOTA fails because no quota is configured.  UFS already uses EINVAL in this case, and it is in fact documented (earlier I claimed it wasn't):

     [EINVAL]           The cmd argument or the command type is invalid.  In
                        Q_GETQUOTASIZE, Q_GETQUOTA, Q_SETQUOTA, and Q_SETUSE,
                        quotas are not currently enabled for this file system.

We should simply modify ZFS to match the documentation.

Comment 11 Sean Eric Fagan 2019-01-11 00:09:55 UTC

Created attachment 201007 [details]
Return EINVAL if no quotas are present

Obviously I have no objection to this ;).

Comment 12 Mark Johnston 2019-01-11 00:54:50 UTC

(In reply to Sean Eric Fagan from comment #11)
LGTM, thanks.

Comment 13 Sean Eric Fagan 2019-01-11 02:54:17 UTC

Just committed as r342928.

Comment 14 commit-hook 2019-01-31 22:07:24 UTC

A commit references this bug:

Author: sef
Date: Thu Jan 31 22:06:47 UTC 2019
New revision: 343623
URL: https://svnweb.freebsd.org/changeset/base/343623

Log:
  MFC r342928:
     Change ZFS quotas to return EINVAL when not present (matches man page).

  Approved by:	mav
  Reviewed by:	markj
  PR:	234413
  Sponsored by:	iXsystems Inc
  Reported by:	Emrion <kmachine@free.fr>

Changes:
_U  stable/12/
  stable/12/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c

Comment 15 commit-hook 2019-01-31 22:08:29 UTC

A commit references this bug:

Author: sef
Date: Thu Jan 31 22:08:02 UTC 2019
New revision: 343624
URL: https://svnweb.freebsd.org/changeset/base/343624

Log:
  MFC r342928:
     Change ZFS quotas to return EINVAL when not present (matches man page).

  Approved by:	mav
  Reviewed by:	markj
  PR:	234413
  Sponsored by:	iXsystems Inc
  Reported by:	Emrion <kmachine@free.fr>

Changes:
_U  stable/11/
  stable/11/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c

Comment 16 Timur I. Bakeyev 2019-02-02 00:28:54 UTC

Sean, so would it be sufficient to put in tho the client code something like:

+#if defined(__FreeBSD__) && ((__FreeBSD_version >= 1102503 && __FreeBSD_version <= 1102506) || (__FreeBSD_version >= 1200072 && __FreeBSD_version <= 1200503) || (__FreeBSD_version >= 1300000 && __FreeBSD_version <= 1300009))
+                       && errno != ENOENT
+#endif

Comment 17 Sean Eric Fagan 2019-02-02 00:31:22 UTC

If you needed to test for that specific error case, that seems sufficient.  But you'd also only want to check for that on a ZFS data set.

Comment 18 Kubilay Kocak 2019-12-15 11:49:58 UTC

^Triage:

- Assign to committer that resolved (sef)
- Affects some people (Specific FreeBSD version and filesystem ZFS)
- Regression between 11.x -> 12.x
- Track merges to stable 12/11