| Message ID | 1453487136-12681-3-git-send-email-sandeen@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri 22-01-16 12:25:31, Eric Sandeen wrote: > Q_XGETNEXTQUOTA is exactly like Q_XGETQUOTA, except that it > will return quota information for the id equal to or greater > than the id requested. In other words, if the requested id has > no quota, the command will return quota information for the > next higher id which does have a quota set. If no higher id > has an active quota, -ESRCH is returned. > > This allows filesystems to do efficient iteration in kernelspace, > much like extN filesystems do in userspace when asked to report > all active quotas. > > The patch adds a d_id field to struct qc_dqblk so that we can > pass back the id of the quota which was found, and return it > to userspace. This paragraph is no longer true. Otherwise the patch looks good to me. Honza > > Today, filesystems such as XFS require getpwent-style iterations, > and for systems which have i.e. LDAP backends, this can be very > slow, or even impossible if iteration is not allowed in the > configuration. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > fs/quota/quota.c | 31 +++++++++++++++++++++++++++++++ > include/linux/quota.h | 2 ++ > include/uapi/linux/dqblk_xfs.h | 1 + > 3 files changed, 34 insertions(+), 0 deletions(-) > > diff --git a/fs/quota/quota.c b/fs/quota/quota.c > index ea66670..0a6dd71 100644 > --- a/fs/quota/quota.c > +++ b/fs/quota/quota.c > @@ -625,6 +625,34 @@ static int quota_getxquota(struct super_block *sb, int type, qid_t id, > return ret; > } > > +/* > + * Return quota for next active quota >= this id, if any exists, > + * otherwise return -ESRCH via ->get_nextdqblk. > + */ > +static int quota_getnextxquota(struct super_block *sb, int type, qid_t id, > + void __user *addr) > +{ > + struct fs_disk_quota fdq; > + struct qc_dqblk qdq; > + struct kqid qid; > + qid_t id_out; > + int ret; > + > + if (!sb->s_qcop->get_nextdqblk) > + return -ENOSYS; > + qid = make_kqid(current_user_ns(), type, id); > + if (!qid_valid(qid)) > + return -EINVAL; > + ret = sb->s_qcop->get_nextdqblk(sb, &qid, &qdq); > + if (ret) > + return ret; > + id_out = from_kqid(current_user_ns(), qid); > + copy_to_xfs_dqblk(&fdq, &qdq, type, id_out); > + if (copy_to_user(addr, &fdq, sizeof(fdq))) > + return -EFAULT; > + return ret; > +} > + > static int quota_rmxquota(struct super_block *sb, void __user *addr) > { > __u32 flags; > @@ -690,6 +718,8 @@ static int do_quotactl(struct super_block *sb, int type, int cmd, qid_t id, > return quota_setxquota(sb, type, id, addr); > case Q_XGETQUOTA: > return quota_getxquota(sb, type, id, addr); > + case Q_XGETNEXTQUOTA: > + return quota_getnextxquota(sb, type, id, addr); > case Q_XQUOTASYNC: > if (sb->s_flags & MS_RDONLY) > return -EROFS; > @@ -712,6 +742,7 @@ static int quotactl_cmd_write(int cmd) > case Q_XGETQSTAT: > case Q_XGETQSTATV: > case Q_XGETQUOTA: > + case Q_XGETNEXTQUOTA: > case Q_XQUOTASYNC: > return 0; > } > diff --git a/include/linux/quota.h b/include/linux/quota.h > index b2505ac..fba92f5 100644 > --- a/include/linux/quota.h > +++ b/include/linux/quota.h > @@ -425,6 +425,8 @@ struct quotactl_ops { > int (*quota_sync)(struct super_block *, int); > int (*set_info)(struct super_block *, int, struct qc_info *); > int (*get_dqblk)(struct super_block *, struct kqid, struct qc_dqblk *); > + int (*get_nextdqblk)(struct super_block *, struct kqid *, > + struct qc_dqblk *); > int (*set_dqblk)(struct super_block *, struct kqid, struct qc_dqblk *); > int (*get_state)(struct super_block *, struct qc_state *); > int (*rm_xquota)(struct super_block *, unsigned int); > diff --git a/include/uapi/linux/dqblk_xfs.h b/include/uapi/linux/dqblk_xfs.h > index dcd75cc..11b3b31 100644 > --- a/include/uapi/linux/dqblk_xfs.h > +++ b/include/uapi/linux/dqblk_xfs.h > @@ -39,6 +39,7 @@ > #define Q_XQUOTARM XQM_CMD(6) /* free disk space used by dquots */ > #define Q_XQUOTASYNC XQM_CMD(7) /* delalloc flush, updates dquots */ > #define Q_XGETQSTATV XQM_CMD(8) /* newer version of get quota */ > +#define Q_XGETNEXTQUOTA XQM_CMD(9) /* get disk limits and usage >= ID */ > > /* > * fs_disk_quota structure: > -- > 1.7.1 >
On Fri 22-01-16 12:25:31, Eric Sandeen wrote: > Q_XGETNEXTQUOTA is exactly like Q_XGETQUOTA, except that it > will return quota information for the id equal to or greater > than the id requested. In other words, if the requested id has > no quota, the command will return quota information for the > next higher id which does have a quota set. If no higher id > has an active quota, -ESRCH is returned. Actually, is -ESRCH the right return value? It seems XFS traditionally returns -ENOENT when id doesn't exist. So that would look more logical to me. Honza > This allows filesystems to do efficient iteration in kernelspace, > much like extN filesystems do in userspace when asked to report > all active quotas. > > The patch adds a d_id field to struct qc_dqblk so that we can > pass back the id of the quota which was found, and return it > to userspace. > > Today, filesystems such as XFS require getpwent-style iterations, > and for systems which have i.e. LDAP backends, this can be very > slow, or even impossible if iteration is not allowed in the > configuration. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > fs/quota/quota.c | 31 +++++++++++++++++++++++++++++++ > include/linux/quota.h | 2 ++ > include/uapi/linux/dqblk_xfs.h | 1 + > 3 files changed, 34 insertions(+), 0 deletions(-) > > diff --git a/fs/quota/quota.c b/fs/quota/quota.c > index ea66670..0a6dd71 100644 > --- a/fs/quota/quota.c > +++ b/fs/quota/quota.c > @@ -625,6 +625,34 @@ static int quota_getxquota(struct super_block *sb, int type, qid_t id, > return ret; > } > > +/* > + * Return quota for next active quota >= this id, if any exists, > + * otherwise return -ESRCH via ->get_nextdqblk. > + */ > +static int quota_getnextxquota(struct super_block *sb, int type, qid_t id, > + void __user *addr) > +{ > + struct fs_disk_quota fdq; > + struct qc_dqblk qdq; > + struct kqid qid; > + qid_t id_out; > + int ret; > + > + if (!sb->s_qcop->get_nextdqblk) > + return -ENOSYS; > + qid = make_kqid(current_user_ns(), type, id); > + if (!qid_valid(qid)) > + return -EINVAL; > + ret = sb->s_qcop->get_nextdqblk(sb, &qid, &qdq); > + if (ret) > + return ret; > + id_out = from_kqid(current_user_ns(), qid); > + copy_to_xfs_dqblk(&fdq, &qdq, type, id_out); > + if (copy_to_user(addr, &fdq, sizeof(fdq))) > + return -EFAULT; > + return ret; > +} > + > static int quota_rmxquota(struct super_block *sb, void __user *addr) > { > __u32 flags; > @@ -690,6 +718,8 @@ static int do_quotactl(struct super_block *sb, int type, int cmd, qid_t id, > return quota_setxquota(sb, type, id, addr); > case Q_XGETQUOTA: > return quota_getxquota(sb, type, id, addr); > + case Q_XGETNEXTQUOTA: > + return quota_getnextxquota(sb, type, id, addr); > case Q_XQUOTASYNC: > if (sb->s_flags & MS_RDONLY) > return -EROFS; > @@ -712,6 +742,7 @@ static int quotactl_cmd_write(int cmd) > case Q_XGETQSTAT: > case Q_XGETQSTATV: > case Q_XGETQUOTA: > + case Q_XGETNEXTQUOTA: > case Q_XQUOTASYNC: > return 0; > } > diff --git a/include/linux/quota.h b/include/linux/quota.h > index b2505ac..fba92f5 100644 > --- a/include/linux/quota.h > +++ b/include/linux/quota.h > @@ -425,6 +425,8 @@ struct quotactl_ops { > int (*quota_sync)(struct super_block *, int); > int (*set_info)(struct super_block *, int, struct qc_info *); > int (*get_dqblk)(struct super_block *, struct kqid, struct qc_dqblk *); > + int (*get_nextdqblk)(struct super_block *, struct kqid *, > + struct qc_dqblk *); > int (*set_dqblk)(struct super_block *, struct kqid, struct qc_dqblk *); > int (*get_state)(struct super_block *, struct qc_state *); > int (*rm_xquota)(struct super_block *, unsigned int); > diff --git a/include/uapi/linux/dqblk_xfs.h b/include/uapi/linux/dqblk_xfs.h > index dcd75cc..11b3b31 100644 > --- a/include/uapi/linux/dqblk_xfs.h > +++ b/include/uapi/linux/dqblk_xfs.h > @@ -39,6 +39,7 @@ > #define Q_XQUOTARM XQM_CMD(6) /* free disk space used by dquots */ > #define Q_XQUOTASYNC XQM_CMD(7) /* delalloc flush, updates dquots */ > #define Q_XGETQSTATV XQM_CMD(8) /* newer version of get quota */ > +#define Q_XGETNEXTQUOTA XQM_CMD(9) /* get disk limits and usage >= ID */ > > /* > * fs_disk_quota structure: > -- > 1.7.1 >
On 1/26/16 6:57 AM, Jan Kara wrote: > On Fri 22-01-16 12:25:31, Eric Sandeen wrote: >> Q_XGETNEXTQUOTA is exactly like Q_XGETQUOTA, except that it >> will return quota information for the id equal to or greater >> than the id requested. In other words, if the requested id has >> no quota, the command will return quota information for the >> next higher id which does have a quota set. If no higher id >> has an active quota, -ESRCH is returned. > > Actually, is -ESRCH the right return value? It seems XFS traditionally > returns -ENOENT when id doesn't exist. So that would look more logical to > me. Hm, I was just going by the quotactl manpage, TBH, which says: ESRCH No disc quota is found for the indicated user. But yes, you are right, it is ENOENT for xfs... argh. I suppose the quotactl manpage could use an update as well, then. At this point do you want me to update the patches & resend or do you want to fix that up too? :( I reference -ESRCH in comments too, I think. -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 26-01-16 09:00:25, Eric Sandeen wrote: > On 1/26/16 6:57 AM, Jan Kara wrote: > > On Fri 22-01-16 12:25:31, Eric Sandeen wrote: > >> Q_XGETNEXTQUOTA is exactly like Q_XGETQUOTA, except that it > >> will return quota information for the id equal to or greater > >> than the id requested. In other words, if the requested id has > >> no quota, the command will return quota information for the > >> next higher id which does have a quota set. If no higher id > >> has an active quota, -ESRCH is returned. > > > > Actually, is -ESRCH the right return value? It seems XFS traditionally > > returns -ENOENT when id doesn't exist. So that would look more logical to > > me. > > Hm, I was just going by the quotactl manpage, TBH, which says: > > ESRCH No disc quota is found for the indicated user. > > > But yes, you are right, it is ENOENT for xfs... argh. I suppose the > quotactl manpage could use an update as well, then. Yeah, so VFS quotas use ESRCH when quota for particular fs is not enabled (while ENOENT means device you passed in doesn't exist). So probably a solution that keeps XFS and VFS interfaces most selfconsistent is to return ENOENT from Q_XGETNEXTQUOTA and ESRCH from Q_GETNEXTQUOTA. I'll update your patches in this sense in the comments and changelogs. But XFS patches (which I don't carry) need updating the actual code... Honza
On 1/26/16 11:52 AM, Jan Kara wrote: > On Tue 26-01-16 09:00:25, Eric Sandeen wrote: >> On 1/26/16 6:57 AM, Jan Kara wrote: >>> On Fri 22-01-16 12:25:31, Eric Sandeen wrote: >>>> Q_XGETNEXTQUOTA is exactly like Q_XGETQUOTA, except that it >>>> will return quota information for the id equal to or greater >>>> than the id requested. In other words, if the requested id has >>>> no quota, the command will return quota information for the >>>> next higher id which does have a quota set. If no higher id >>>> has an active quota, -ESRCH is returned. >>> >>> Actually, is -ESRCH the right return value? It seems XFS traditionally >>> returns -ENOENT when id doesn't exist. So that would look more logical to >>> me. >> >> Hm, I was just going by the quotactl manpage, TBH, which says: >> >> ESRCH No disc quota is found for the indicated user. >> >> >> But yes, you are right, it is ENOENT for xfs... argh. I suppose the >> quotactl manpage could use an update as well, then. > > Yeah, so VFS quotas use ESRCH when quota for particular fs is not enabled > (while ENOENT means device you passed in doesn't exist). So probably a > solution that keeps XFS and VFS interfaces most selfconsistent is to return > ENOENT from Q_XGETNEXTQUOTA and ESRCH from Q_GETNEXTQUOTA. I'll update your > patches in this sense in the comments and changelogs. But XFS patches > (which I don't carry) need updating the actual code... *nod* thanks. I'll just resend the XFS patches to the XFS list. -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1/26/16 11:52 AM, Jan Kara wrote: > Yeah, so VFS quotas use ESRCH when quota for particular fs is not enabled > (while ENOENT means device you passed in doesn't exist). And what does it return for "that ID has no quota?" Anything? Maybe not, see below? > So probably a > solution that keeps XFS and VFS interfaces most selfconsistent is to return > ENOENT from Q_XGETNEXTQUOTA and ESRCH from Q_GETNEXTQUOTA. I'll update your > patches in this sense in the comments and changelogs. But XFS patches > (which I don't carry) need updating the actual code... Actually, ok, now I'm a little more confused. Today, Q_XGETQUOTA and Q_GETQUOTA will both return -ENOENT on XFS if a quota doesn't exist for that ID: quotactl(Q_GETQUOTA|USRQUOTA, "/dev/sdb2", 123456789, 0x7ffdf2edbc80) = -1 ENOENT (No such file or directory) quotactl(Q_XGETQUOTA|USRQUOTA, "/dev/sdb2", 123456789, 0x7ffdf2edbc10) = -1 ENOENT (No such file or directory) It seems like the *NEXT* variants (on xfs at least?) may as well continue to do the same... But on ext4, I see 0 returned for a nonexistent quota: quotactl(Q_GETQUOTA|USRQUOTA, "/dev/sdb1", 123456789, {bhardlimit=0, bsoftlimit=0, curspace=0, ihardlimit=0, isoftlimit=0, curinodes=0, ...}) = 0 quotactl(Q_XGETQUOTA|USRQUOTA, "/dev/sdb1", 123456789, {version=1, flags=XFS_USER_QUOTA, fieldmask=0, id=123456789, blk_hardlimit=0, blk_softlimit=0, ino_hardlimit=0, ino_softlimit=0, bcount=0, icount=0, ...}) = 0 So the difference doesn't seem to be XGETQUOTA vs GETQUOTA, rather it's the filesystem handling the call? Still, we do need a way to pass back "No more quotas to find" from Q_[X]GETNEXTQUOTA; XFS will do -ENOENT, but if -ENOENT and -ESRCH already have specific meanings on non-xfs filesystems, I'm not sure where we go from there. Sorry for my confusion. :( -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 26-01-16 12:39:59, Eric Sandeen wrote: > On 1/26/16 11:52 AM, Jan Kara wrote: > > > Yeah, so VFS quotas use ESRCH when quota for particular fs is not enabled > > (while ENOENT means device you passed in doesn't exist). > > And what does it return for "that ID has no quota?" Anything? Maybe not, > see below? > > > So probably a > > solution that keeps XFS and VFS interfaces most selfconsistent is to return > > ENOENT from Q_XGETNEXTQUOTA and ESRCH from Q_GETNEXTQUOTA. I'll update your > > patches in this sense in the comments and changelogs. But XFS patches > > (which I don't carry) need updating the actual code... > > Actually, ok, now I'm a little more confused. > > Today, Q_XGETQUOTA and Q_GETQUOTA will both return -ENOENT on XFS if a > quota doesn't exist for that ID: > > quotactl(Q_GETQUOTA|USRQUOTA, "/dev/sdb2", 123456789, 0x7ffdf2edbc80) = -1 ENOENT (No such file or directory) > quotactl(Q_XGETQUOTA|USRQUOTA, "/dev/sdb2", 123456789, 0x7ffdf2edbc10) = -1 ENOENT (No such file or directory) > > It seems like the *NEXT* variants (on xfs at least?) may as well continue > to do the same... > > But on ext4, I see 0 returned for a nonexistent quota: > > quotactl(Q_GETQUOTA|USRQUOTA, "/dev/sdb1", 123456789, {bhardlimit=0, bsoftlimit=0, curspace=0, ihardlimit=0, isoftlimit=0, curinodes=0, ...}) = 0 > quotactl(Q_XGETQUOTA|USRQUOTA, "/dev/sdb1", 123456789, {version=1, flags=XFS_USER_QUOTA, fieldmask=0, id=123456789, blk_hardlimit=0, blk_softlimit=0, ino_hardlimit=0, ino_softlimit=0, bcount=0, icount=0, ...}) = 0 > > So the difference doesn't seem to be XGETQUOTA vs GETQUOTA, rather it's > the filesystem handling the call? Yes, it's about whether fs/quota/dquot.c or XFS handles the call. VFS traditionally returns structure full of zeros when you call Q_GETQUOTA for non-existent ID on a system that otherwise supports quotas (that is a historical heritage of old quota file format). This is however impractical for Q_GETNEXTQUOTA so there we have to define some error terminating the iteration. > Still, we do need a way to pass back "No more quotas to find" from > Q_[X]GETNEXTQUOTA; XFS will do -ENOENT, but if -ENOENT and -ESRCH already > have specific meanings on non-xfs filesystems, I'm not sure where we go > from there. Yeah, we have to simply overload one of the error codes for VFS implementation and I'm not strongly convinced which one. But since you pointed out that it's not really about the interface but about backing fs, I think making both ENOENT is probably going to be less confusing for userspace. Honza
diff --git a/fs/quota/quota.c b/fs/quota/quota.c index ea66670..0a6dd71 100644 --- a/fs/quota/quota.c +++ b/fs/quota/quota.c @@ -625,6 +625,34 @@ static int quota_getxquota(struct super_block *sb, int type, qid_t id, return ret; } +/* + * Return quota for next active quota >= this id, if any exists, + * otherwise return -ESRCH via ->get_nextdqblk. + */ +static int quota_getnextxquota(struct super_block *sb, int type, qid_t id, + void __user *addr) +{ + struct fs_disk_quota fdq; + struct qc_dqblk qdq; + struct kqid qid; + qid_t id_out; + int ret; + + if (!sb->s_qcop->get_nextdqblk) + return -ENOSYS; + qid = make_kqid(current_user_ns(), type, id); + if (!qid_valid(qid)) + return -EINVAL; + ret = sb->s_qcop->get_nextdqblk(sb, &qid, &qdq); + if (ret) + return ret; + id_out = from_kqid(current_user_ns(), qid); + copy_to_xfs_dqblk(&fdq, &qdq, type, id_out); + if (copy_to_user(addr, &fdq, sizeof(fdq))) + return -EFAULT; + return ret; +} + static int quota_rmxquota(struct super_block *sb, void __user *addr) { __u32 flags; @@ -690,6 +718,8 @@ static int do_quotactl(struct super_block *sb, int type, int cmd, qid_t id, return quota_setxquota(sb, type, id, addr); case Q_XGETQUOTA: return quota_getxquota(sb, type, id, addr); + case Q_XGETNEXTQUOTA: + return quota_getnextxquota(sb, type, id, addr); case Q_XQUOTASYNC: if (sb->s_flags & MS_RDONLY) return -EROFS; @@ -712,6 +742,7 @@ static int quotactl_cmd_write(int cmd) case Q_XGETQSTAT: case Q_XGETQSTATV: case Q_XGETQUOTA: + case Q_XGETNEXTQUOTA: case Q_XQUOTASYNC: return 0; } diff --git a/include/linux/quota.h b/include/linux/quota.h index b2505ac..fba92f5 100644 --- a/include/linux/quota.h +++ b/include/linux/quota.h @@ -425,6 +425,8 @@ struct quotactl_ops { int (*quota_sync)(struct super_block *, int); int (*set_info)(struct super_block *, int, struct qc_info *); int (*get_dqblk)(struct super_block *, struct kqid, struct qc_dqblk *); + int (*get_nextdqblk)(struct super_block *, struct kqid *, + struct qc_dqblk *); int (*set_dqblk)(struct super_block *, struct kqid, struct qc_dqblk *); int (*get_state)(struct super_block *, struct qc_state *); int (*rm_xquota)(struct super_block *, unsigned int); diff --git a/include/uapi/linux/dqblk_xfs.h b/include/uapi/linux/dqblk_xfs.h index dcd75cc..11b3b31 100644 --- a/include/uapi/linux/dqblk_xfs.h +++ b/include/uapi/linux/dqblk_xfs.h @@ -39,6 +39,7 @@ #define Q_XQUOTARM XQM_CMD(6) /* free disk space used by dquots */ #define Q_XQUOTASYNC XQM_CMD(7) /* delalloc flush, updates dquots */ #define Q_XGETQSTATV XQM_CMD(8) /* newer version of get quota */ +#define Q_XGETNEXTQUOTA XQM_CMD(9) /* get disk limits and usage >= ID */ /* * fs_disk_quota structure:
Q_XGETNEXTQUOTA is exactly like Q_XGETQUOTA, except that it will return quota information for the id equal to or greater than the id requested. In other words, if the requested id has no quota, the command will return quota information for the next higher id which does have a quota set. If no higher id has an active quota, -ESRCH is returned. This allows filesystems to do efficient iteration in kernelspace, much like extN filesystems do in userspace when asked to report all active quotas. The patch adds a d_id field to struct qc_dqblk so that we can pass back the id of the quota which was found, and return it to userspace. Today, filesystems such as XFS require getpwent-style iterations, and for systems which have i.e. LDAP backends, this can be very slow, or even impossible if iteration is not allowed in the configuration. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- fs/quota/quota.c | 31 +++++++++++++++++++++++++++++++ include/linux/quota.h | 2 ++ include/uapi/linux/dqblk_xfs.h | 1 + 3 files changed, 34 insertions(+), 0 deletions(-)