* [PATCH 0/4] quota: add project quota support @ 2014-09-24 14:04 Li Xi 2014-09-24 14:04 ` [PATCH 1/4] Adds general codes to enforces project quota limits Li Xi ` (3 more replies) 0 siblings, 4 replies; 41+ messages in thread From: Li Xi @ 2014-09-24 14:04 UTC (permalink / raw) To: linux-fsdevel, linux-ext4, linux-api, tytso, adilger, jack, viro, hch, dmonakhov The following patches propose an implementation of project quota support for ext4. A project is an aggregate of unrelated inodes which might scatter in different directories. Inodes belongs to a project possesses a same identification i.e. 'project ID', just like every inode has its user/group identification. The following patches adds project quota as supplement to the former uer/group quota types. The semantics of ext4 project quota is consistent with XFS. Each directory can have EXT4_INODE_PROJINHERIT flag set. When the EXT4_INODE_PROJINHERIT flag of a parent directory is not set, a newly created inode under that directory will have a default project ID (i.e. 0). And its EXT4_INODE_PROJINHERIT flag is not set either. When this flag is set on a directory, following rules will be kept: 1) The newly created inode under that directory will inherit both the EXT4_INODE_PROJINHERIT flag and the project ID from its parent directory. 2) Hard-linking a inode with different project ID into that directory will fail with errno EXDEV. 3) Renaming a inode with different project ID into that directory will fail with errno EXDEV. However, 'mv' command will detect this failure and copy the renamed inode to a new inode in the directory. Thus, this new inode will inherit both the project ID and EXT4_INODE_PROJINHERIT flag. 4) If the project quota of that ID is being enforced, statfs() on that directory will take the quotas as another upper limits along with the capacity of the file system, i.e. the total block/inode number will be the minimum of the quota limits and file system capacity. Changelog: * v4 <- v3: - Do not check project feature when set/get project ID; - Use EXT4_MAXQUOTAS instead of MAXQUOTAS in ext4 patches; - Remove unnecessary change of fs/quota/dquot.c; - Remove CONFIG_QUOTA_PROJECT. * v3 <- v2: - Add EXT4_INODE_PROJINHERIT semantics. * v2 <- v1: - Add ioctl interface for setting/getting project; - Add EXT4_FEATURE_RO_COMPAT_PROJECT; - Add get_projid() method in struct dquot_operations; - Add error check of ext4_inode_projid_set/get(). v3: http://www.spinics.net/lists/linux-ext4/msg45184.html v2: http://www.spinics.net/lists/linux-ext4/msg44695.html v1: http://article.gmane.org/gmane.comp.file-systems.ext4/45153 Any comments or feedbacks are appreciated. Regards, - Li Xi Li Xi (4): Adds general codes to enforces project quota limits Adds project ID support for ext4 Adds project quota support for ext4 Adds ioctl interface support for ext4 project Documentation/filesystems/ext4.txt | 4 + fs/ext4/ext4.h | 31 +++++++-- fs/ext4/ialloc.c | 6 ++ fs/ext4/inode.c | 29 ++++++++- fs/ext4/ioctl.c | 85 ++++++++++++++++++++++++ fs/ext4/namei.c | 10 +++ fs/ext4/super.c | 124 ++++++++++++++++++++++++++++++++--- fs/quota/dquot.c | 16 ++++- fs/quota/quota.c | 5 +- fs/quota/quotaio_v2.h | 6 +- include/linux/quota.h | 2 + include/uapi/linux/quota.h | 6 +- 12 files changed, 299 insertions(+), 25 deletions(-) ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/4] Adds general codes to enforces project quota limits 2014-09-24 14:04 [PATCH 0/4] quota: add project quota support Li Xi @ 2014-09-24 14:04 ` Li Xi 2014-09-24 16:08 ` Jan Kara [not found] ` <1411567470-31799-2-git-send-email-lixi-LfVdkaOWEx8@public.gmane.org> 2014-09-24 14:04 ` [PATCH 2/4] Adds project ID support for ext4 Li Xi ` (2 subsequent siblings) 3 siblings, 2 replies; 41+ messages in thread From: Li Xi @ 2014-09-24 14:04 UTC (permalink / raw) To: linux-fsdevel, linux-ext4, linux-api, tytso, adilger, jack, viro, hch, dmonakhov This patch adds support for a new quota type PRJQUOTA for project quota enforcement. Also a new method get_projid() is added into dquot_operations structure. Signed-off-by: Li Xi <lixi@ddn.com> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org> --- fs/quota/dquot.c | 16 ++++++++++++++-- fs/quota/quota.c | 5 ++++- fs/quota/quotaio_v2.h | 6 ++++-- include/linux/quota.h | 2 ++ include/uapi/linux/quota.h | 6 ++++-- 5 files changed, 28 insertions(+), 7 deletions(-) diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c index f2d0eee..8d0c270 100644 --- a/fs/quota/dquot.c +++ b/fs/quota/dquot.c @@ -1154,8 +1154,8 @@ static int need_print_warning(struct dquot_warn *warn) return uid_eq(current_fsuid(), warn->w_dq_id.uid); case GRPQUOTA: return in_group_p(warn->w_dq_id.gid); - case PRJQUOTA: /* Never taken... Just make gcc happy */ - return 0; + case PRJQUOTA: + return 1; } return 0; } @@ -1394,6 +1394,9 @@ static void __dquot_initialize(struct inode *inode, int type) /* First get references to structures we might need. */ for (cnt = 0; cnt < MAXQUOTAS; cnt++) { struct kqid qid; + kprojid_t projid; + int rc; + got[cnt] = NULL; if (type != -1 && cnt != type) continue; @@ -1413,6 +1416,15 @@ static void __dquot_initialize(struct inode *inode, int type) case GRPQUOTA: qid = make_kqid_gid(inode->i_gid); break; + case PRJQUOTA: + /* Project ID is not supported */ + if (!inode->i_sb->dq_op->get_projid) + continue; + rc = inode->i_sb->dq_op->get_projid(inode, &projid); + if (rc) + continue; + qid = make_kqid_projid(projid); + break; } got[cnt] = dqget(sb, qid); } diff --git a/fs/quota/quota.c b/fs/quota/quota.c index 7562164..cce7371 100644 --- a/fs/quota/quota.c +++ b/fs/quota/quota.c @@ -30,7 +30,10 @@ static int check_quotactl_permission(struct super_block *sb, int type, int cmd, case Q_XGETQSTATV: case Q_XQUOTASYNC: break; - /* allow to query information for dquots we "own" */ + /* + * allow to query information for dquots we "own" + * always allow quota check for project quota + */ case Q_GETQUOTA: case Q_XGETQUOTA: if ((type == USRQUOTA && uid_eq(current_euid(), make_kuid(current_user_ns(), id))) || diff --git a/fs/quota/quotaio_v2.h b/fs/quota/quotaio_v2.h index f1966b4..b17af05 100644 --- a/fs/quota/quotaio_v2.h +++ b/fs/quota/quotaio_v2.h @@ -13,12 +13,14 @@ */ #define V2_INITQMAGICS {\ 0xd9c01f11, /* USRQUOTA */\ - 0xd9c01927 /* GRPQUOTA */\ + 0xd9c01927, /* GRPQUOTA */\ + 0xd9c03f14 /* PRJQUOTA */\ } #define V2_INITQVERSIONS {\ 1, /* USRQUOTA */\ - 1 /* GRPQUOTA */\ + 1, /* GRPQUOTA */\ + 1 /* PRJQUOTA */\ } /* First generic header */ diff --git a/include/linux/quota.h b/include/linux/quota.h index 80d345a..f1b25f8 100644 --- a/include/linux/quota.h +++ b/include/linux/quota.h @@ -50,6 +50,7 @@ #undef USRQUOTA #undef GRPQUOTA +#undef PRJQUOTA enum quota_type { USRQUOTA = 0, /* element used for user quotas */ GRPQUOTA = 1, /* element used for group quotas */ @@ -312,6 +313,7 @@ struct dquot_operations { /* get reserved quota for delayed alloc, value returned is managed by * quota code only */ qsize_t *(*get_reserved_space) (struct inode *); + int (*get_projid) (struct inode *, kprojid_t *);/* Get project ID */ }; struct path; diff --git a/include/uapi/linux/quota.h b/include/uapi/linux/quota.h index 3b6cfbe..b2d9486 100644 --- a/include/uapi/linux/quota.h +++ b/include/uapi/linux/quota.h @@ -36,11 +36,12 @@ #include <linux/errno.h> #include <linux/types.h> -#define __DQUOT_VERSION__ "dquot_6.5.2" +#define __DQUOT_VERSION__ "dquot_6.6.0" -#define MAXQUOTAS 2 +#define MAXQUOTAS 3 #define USRQUOTA 0 /* element used for user quotas */ #define GRPQUOTA 1 /* element used for group quotas */ +#define PRJQUOTA 2 /* element used for project quotas */ /* * Definitions for the default names of the quotas files. @@ -48,6 +49,7 @@ #define INITQFNAMES { \ "user", /* USRQUOTA */ \ "group", /* GRPQUOTA */ \ + "project", /* PRJQUOTA */ \ "undefined", \ }; -- 1.7.1 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH 1/4] Adds general codes to enforces project quota limits 2014-09-24 14:04 ` [PATCH 1/4] Adds general codes to enforces project quota limits Li Xi @ 2014-09-24 16:08 ` Jan Kara [not found] ` <1411567470-31799-2-git-send-email-lixi-LfVdkaOWEx8@public.gmane.org> 1 sibling, 0 replies; 41+ messages in thread From: Jan Kara @ 2014-09-24 16:08 UTC (permalink / raw) To: Li Xi Cc: linux-fsdevel, linux-ext4, linux-api, tytso, adilger, jack, viro, hch, dmonakhov On Wed 24-09-14 22:04:27, Li Xi wrote: > This patch adds support for a new quota type PRJQUOTA for project quota > enforcement. Also a new method get_projid() is added into dquot_operations > structure. > > Signed-off-by: Li Xi <lixi@ddn.com> > Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org> > --- > fs/quota/dquot.c | 16 ++++++++++++++-- > fs/quota/quota.c | 5 ++++- > fs/quota/quotaio_v2.h | 6 ++++-- > include/linux/quota.h | 2 ++ > include/uapi/linux/quota.h | 6 ++++-- > 5 files changed, 28 insertions(+), 7 deletions(-) > > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c > index f2d0eee..8d0c270 100644 > --- a/fs/quota/dquot.c > +++ b/fs/quota/dquot.c > @@ -1154,8 +1154,8 @@ static int need_print_warning(struct dquot_warn *warn) > return uid_eq(current_fsuid(), warn->w_dq_id.uid); > case GRPQUOTA: > return in_group_p(warn->w_dq_id.gid); > - case PRJQUOTA: /* Never taken... Just make gcc happy */ > - return 0; > + case PRJQUOTA: > + return 1; > } > return 0; > } > @@ -1394,6 +1394,9 @@ static void __dquot_initialize(struct inode *inode, int type) > /* First get references to structures we might need. */ > for (cnt = 0; cnt < MAXQUOTAS; cnt++) { > struct kqid qid; > + kprojid_t projid; > + int rc; > + > got[cnt] = NULL; > if (type != -1 && cnt != type) > continue; > @@ -1413,6 +1416,15 @@ static void __dquot_initialize(struct inode *inode, int type) > case GRPQUOTA: > qid = make_kqid_gid(inode->i_gid); > break; > + case PRJQUOTA: > + /* Project ID is not supported */ > + if (!inode->i_sb->dq_op->get_projid) > + continue; > + rc = inode->i_sb->dq_op->get_projid(inode, &projid); > + if (rc) > + continue; > + qid = make_kqid_projid(projid); > + break; > } > got[cnt] = dqget(sb, qid); > } Looking into dquot.c you seem to be missing a change to do_get_dqblk() where setting of di->d_flags needs to handle PRJQUOTA now as well. > diff --git a/fs/quota/quota.c b/fs/quota/quota.c > index 7562164..cce7371 100644 > --- a/fs/quota/quota.c > +++ b/fs/quota/quota.c > @@ -30,7 +30,10 @@ static int check_quotactl_permission(struct super_block *sb, int type, int cmd, > case Q_XGETQSTATV: > case Q_XQUOTASYNC: > break; > - /* allow to query information for dquots we "own" */ > + /* > + * allow to query information for dquots we "own" > + * always allow quota check for project quota The last sentense should probably be: "always allow querying project quota" > + */ > case Q_GETQUOTA: > case Q_XGETQUOTA: > if ((type == USRQUOTA && uid_eq(current_euid(), make_kuid(current_user_ns(), id))) || But looking at the code, you need to add there || type == PRJQUOTA don't you? Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 41+ messages in thread
[parent not found: <1411567470-31799-2-git-send-email-lixi-LfVdkaOWEx8@public.gmane.org>]
* Re: [PATCH 1/4] Adds general codes to enforces project quota limits [not found] ` <1411567470-31799-2-git-send-email-lixi-LfVdkaOWEx8@public.gmane.org> @ 2014-09-24 16:14 ` Christoph Hellwig [not found] ` <20140924161417.GA1978-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> 2014-09-30 20:07 ` Jan Kara 1 sibling, 1 reply; 41+ messages in thread From: Christoph Hellwig @ 2014-09-24 16:14 UTC (permalink / raw) To: Li Xi Cc: linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-ext4-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA, tytso-3s7WtUTddSA, adilger-m1MBpc4rdrD3fQ9qLvQP4Q, jack-AlSwsSmVLrQ, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, hch-wEGCiKHe2LqWVfeAwA7xHQ, dmonakhov-GEFAQzZX7r8dnm+yROfE0A > -#define MAXQUOTAS 2 > +#define MAXQUOTAS 3 This bloats every single inode in the system. I think this would be a good opportunity to move i_dquot into the per-filesystem structures and provide a little get_dquots methods to get at them from the quota code. ^ permalink raw reply [flat|nested] 41+ messages in thread
[parent not found: <20140924161417.GA1978-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>]
* Re: [PATCH 1/4] Adds general codes to enforces project quota limits [not found] ` <20140924161417.GA1978-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> @ 2014-09-24 17:10 ` Jan Kara [not found] ` <20140924171020.GF27000-+0h/O2h83AeN3ZZ/Hiejyg@public.gmane.org> 0 siblings, 1 reply; 41+ messages in thread From: Jan Kara @ 2014-09-24 17:10 UTC (permalink / raw) To: Christoph Hellwig Cc: Li Xi, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-ext4-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA, tytso-3s7WtUTddSA, adilger-m1MBpc4rdrD3fQ9qLvQP4Q, jack-AlSwsSmVLrQ, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, dmonakhov-GEFAQzZX7r8dnm+yROfE0A On Wed 24-09-14 09:14:17, Christoph Hellwig wrote: > > -#define MAXQUOTAS 2 > > +#define MAXQUOTAS 3 > > This bloats every single inode in the system. I think this would be a > good opportunity to move i_dquot into the per-filesystem structures > and provide a little get_dquots methods to get at them from the > quota code. Yeah, I'm aware of that but I decided I won't bother Li Xi with that since it's independent issue and 8 bytes aren't that terrible. Personally I somewhat prefer what I did in http://www.spinics.net/lists/linux-fsdevel/msg74927.html where we don't introduce additional method but rather a table with field offsets in superblock. If people agree with this, I can cook up a patch for quota relatively quickly. Honza -- Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> SUSE Labs, CR ^ permalink raw reply [flat|nested] 41+ messages in thread
[parent not found: <20140924171020.GF27000-+0h/O2h83AeN3ZZ/Hiejyg@public.gmane.org>]
* Re: [PATCH 1/4] Adds general codes to enforces project quota limits [not found] ` <20140924171020.GF27000-+0h/O2h83AeN3ZZ/Hiejyg@public.gmane.org> @ 2014-09-24 17:13 ` Christoph Hellwig [not found] ` <20140924171306.GA23874-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> 0 siblings, 1 reply; 41+ messages in thread From: Christoph Hellwig @ 2014-09-24 17:13 UTC (permalink / raw) To: Jan Kara Cc: Christoph Hellwig, Li Xi, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-ext4-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA, tytso-3s7WtUTddSA, adilger-m1MBpc4rdrD3fQ9qLvQP4Q, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, dmonakhov-GEFAQzZX7r8dnm+yROfE0A On Wed, Sep 24, 2014 at 07:10:20PM +0200, Jan Kara wrote: > Yeah, I'm aware of that but I decided I won't bother Li Xi with that > since it's independent issue and 8 bytes aren't that terrible. For the inode of which we have so many instances it is. Especially when only one filesystem can make use of this new field. > Personally > I somewhat prefer what I did in > http://www.spinics.net/lists/linux-fsdevel/msg74927.html > where we don't introduce additional method but rather a table with field > offsets in superblock. If people agree with this, I can cook up a patch for > quota relatively quickly. There's an even better way. We stopped calling the dquot_* functions from generic code, so now the filesystem can simply pass in a pointer to the dquot array. ^ permalink raw reply [flat|nested] 41+ messages in thread
[parent not found: <20140924171306.GA23874-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>]
* Re: [PATCH 1/4] Adds general codes to enforces project quota limits [not found] ` <20140924171306.GA23874-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> @ 2014-09-24 17:37 ` Jan Kara 0 siblings, 0 replies; 41+ messages in thread From: Jan Kara @ 2014-09-24 17:37 UTC (permalink / raw) To: Christoph Hellwig Cc: Jan Kara, Li Xi, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-ext4-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA, tytso-3s7WtUTddSA, adilger-m1MBpc4rdrD3fQ9qLvQP4Q, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, dmonakhov-GEFAQzZX7r8dnm+yROfE0A On Wed 24-09-14 10:13:06, Christoph Hellwig wrote: > On Wed, Sep 24, 2014 at 07:10:20PM +0200, Jan Kara wrote: > > Yeah, I'm aware of that but I decided I won't bother Li Xi with that > > since it's independent issue and 8 bytes aren't that terrible. > > For the inode of which we have so many instances it is. Especially when > only one filesystem can make use of this new field. > > > Personally > > I somewhat prefer what I did in > > http://www.spinics.net/lists/linux-fsdevel/msg74927.html > > where we don't introduce additional method but rather a table with field > > offsets in superblock. If people agree with this, I can cook up a patch for > > quota relatively quickly. > > There's an even better way. We stopped calling the dquot_* functions > from generic code, so now the filesystem can simply pass in a pointer > to the dquot array. I thought that as well before I tried that :) There are situations like when turning quotas on & off where we iterate over all inodes and need to get dquots from the inode... I don't see how to handle those situations without fs callback / table of offsets. Honza -- Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> SUSE Labs, CR ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/4] Adds general codes to enforces project quota limits [not found] ` <1411567470-31799-2-git-send-email-lixi-LfVdkaOWEx8@public.gmane.org> 2014-09-24 16:14 ` Christoph Hellwig @ 2014-09-30 20:07 ` Jan Kara 1 sibling, 0 replies; 41+ messages in thread From: Jan Kara @ 2014-09-30 20:07 UTC (permalink / raw) To: Li Xi Cc: linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-ext4-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA, tytso-3s7WtUTddSA, adilger-m1MBpc4rdrD3fQ9qLvQP4Q, jack-AlSwsSmVLrQ, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, hch-wEGCiKHe2LqWVfeAwA7xHQ, dmonakhov-GEFAQzZX7r8dnm+yROfE0A On Wed 24-09-14 22:04:27, Li Xi wrote: > @@ -1413,6 +1416,15 @@ static void __dquot_initialize(struct inode *inode, int type) > case GRPQUOTA: > qid = make_kqid_gid(inode->i_gid); > break; > + case PRJQUOTA: > + /* Project ID is not supported */ > + if (!inode->i_sb->dq_op->get_projid) > + continue; > + rc = inode->i_sb->dq_op->get_projid(inode, &projid); > + if (rc) > + continue; > + qid = make_kqid_projid(projid); > + break; I realized one more thing: We don't want to check ->get_projid() here. Rather we should check ->get_projid() when turning quota on in quota_quotaon() and return -EINVAL when ->get_projid() isn't set - that way user is early notified the fs doesn't support project quotas. Then here in __dquot_initialize() we can just check sb_has_quota_active(sb, PRJQUOTA). Honza -- Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> SUSE Labs, CR ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 2/4] Adds project ID support for ext4 2014-09-24 14:04 [PATCH 0/4] quota: add project quota support Li Xi 2014-09-24 14:04 ` [PATCH 1/4] Adds general codes to enforces project quota limits Li Xi @ 2014-09-24 14:04 ` Li Xi 2014-09-24 17:11 ` Jan Kara 2014-09-24 14:04 ` [PATCH 3/4] Adds project quota " Li Xi 2014-09-24 14:04 ` [PATCH 4/4] Adds ioctl interface support for ext4 project Li Xi 3 siblings, 1 reply; 41+ messages in thread From: Li Xi @ 2014-09-24 14:04 UTC (permalink / raw) To: linux-fsdevel, linux-ext4, linux-api, tytso, adilger, jack, viro, hch, dmonakhov This patch adds a new internal field of ext4 inode to save project identifier. Also a new flag EXT4_INODE_PROJINHERIT is added for inheriting project ID from parent directory. Signed-off-by: Li Xi <lixi@ddn.com> --- fs/ext4/ext4.h | 21 +++++++++++++++++---- fs/ext4/ialloc.c | 6 ++++++ fs/ext4/inode.c | 29 ++++++++++++++++++++++++++++- fs/ext4/namei.c | 10 ++++++++++ fs/ext4/super.c | 1 + 5 files changed, 62 insertions(+), 5 deletions(-) diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 1eb5b7b..092f60c 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -386,16 +386,18 @@ struct flex_groups { #define EXT4_EA_INODE_FL 0x00200000 /* Inode used for large EA */ #define EXT4_EOFBLOCKS_FL 0x00400000 /* Blocks allocated beyond EOF */ #define EXT4_INLINE_DATA_FL 0x10000000 /* Inode has inline data. */ +#define EXT4_PROJINHERIT_FL 0x20000000 /* Create with parents projid */ #define EXT4_RESERVED_FL 0x80000000 /* reserved for ext4 lib */ -#define EXT4_FL_USER_VISIBLE 0x004BDFFF /* User visible flags */ -#define EXT4_FL_USER_MODIFIABLE 0x004380FF /* User modifiable flags */ +#define EXT4_FL_USER_VISIBLE 0x204BDFFF /* User visible flags */ +#define EXT4_FL_USER_MODIFIABLE 0x204380FF /* User modifiable flags */ /* Flags that should be inherited by new inodes from their parent. */ #define EXT4_FL_INHERITED (EXT4_SECRM_FL | EXT4_UNRM_FL | EXT4_COMPR_FL |\ EXT4_SYNC_FL | EXT4_NODUMP_FL | EXT4_NOATIME_FL |\ EXT4_NOCOMPR_FL | EXT4_JOURNAL_DATA_FL |\ - EXT4_NOTAIL_FL | EXT4_DIRSYNC_FL) + EXT4_NOTAIL_FL | EXT4_DIRSYNC_FL |\ + EXT4_PROJINHERIT_FL) /* Flags that are appropriate for regular files (all but dir-specific ones). */ #define EXT4_REG_FLMASK (~(EXT4_DIRSYNC_FL | EXT4_TOPDIR_FL)) @@ -443,6 +445,7 @@ enum { EXT4_INODE_EA_INODE = 21, /* Inode used for large EA */ EXT4_INODE_EOFBLOCKS = 22, /* Blocks allocated beyond EOF */ EXT4_INODE_INLINE_DATA = 28, /* Data in inode. */ + EXT4_INODE_PROJINHERIT = 29, /* Create with parents projid */ EXT4_INODE_RESERVED = 31, /* reserved for ext4 lib */ }; @@ -694,6 +697,7 @@ struct ext4_inode { __le32 i_crtime; /* File Creation time */ __le32 i_crtime_extra; /* extra FileCreationtime (nsec << 2 | epoch) */ __le32 i_version_hi; /* high 32 bits for 64-bit version */ + __le32 i_projid; /* Project ID */ }; struct move_extent { @@ -943,6 +947,7 @@ struct ext4_inode_info { /* Precomputed uuid+inum+igen checksum for seeding inode checksums */ __u32 i_csum_seed; + kprojid_t i_projid; }; /* @@ -1526,6 +1531,7 @@ static inline void ext4_clear_state_flags(struct ext4_inode_info *ei) * GDT_CSUM bits are mutually exclusive. */ #define EXT4_FEATURE_RO_COMPAT_METADATA_CSUM 0x0400 +#define EXT4_FEATURE_RO_COMPAT_PROJECT 0x1000 /* Project quota */ #define EXT4_FEATURE_INCOMPAT_COMPRESSION 0x0001 #define EXT4_FEATURE_INCOMPAT_FILETYPE 0x0002 @@ -1575,7 +1581,8 @@ static inline void ext4_clear_state_flags(struct ext4_inode_info *ei) EXT4_FEATURE_RO_COMPAT_HUGE_FILE |\ EXT4_FEATURE_RO_COMPAT_BIGALLOC |\ EXT4_FEATURE_RO_COMPAT_METADATA_CSUM|\ - EXT4_FEATURE_RO_COMPAT_QUOTA) + EXT4_FEATURE_RO_COMPAT_QUOTA |\ + EXT4_FEATURE_RO_COMPAT_PROJECT) /* * Default values for user and/or group using reserved blocks @@ -1583,6 +1590,11 @@ static inline void ext4_clear_state_flags(struct ext4_inode_info *ei) #define EXT4_DEF_RESUID 0 #define EXT4_DEF_RESGID 0 +/* + * Default project ID + */ +#define EXT4_DEF_PROJID 0 + #define EXT4_DEF_INODE_READAHEAD_BLKS 32 /* @@ -2134,6 +2146,7 @@ extern int ext4_zero_partial_blocks(handle_t *handle, struct inode *inode, loff_t lstart, loff_t lend); extern int ext4_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf); extern qsize_t *ext4_get_reserved_space(struct inode *inode); +extern int ext4_get_projid(struct inode *inode, kprojid_t *projid); extern void ext4_da_update_reserve_space(struct inode *inode, int used, int quota_claim); diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c index 5b87fc3..b6b7284 100644 --- a/fs/ext4/ialloc.c +++ b/fs/ext4/ialloc.c @@ -756,6 +756,12 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir, inode->i_gid = dir->i_gid; } else inode_init_owner(inode, dir, mode); + if (ext4_test_inode_flag(dir, EXT4_INODE_PROJINHERIT)) { + ei->i_projid = EXT4_I(dir)->i_projid; + } else { + ei->i_projid = + make_kprojid(&init_user_ns, EXT4_DEF_PROJID); + } dquot_initialize(inode); if (!goal) diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index d5dd7d4..8c641eb 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -3874,6 +3874,24 @@ static inline void ext4_iget_extra_inode(struct inode *inode, EXT4_I(inode)->i_inline_off = 0; } +static projid_t ext4_inode_projid_get(struct inode *inode, struct ext4_inode *raw, + struct ext4_inode_info *ei) +{ + return (projid_t)le32_to_cpu(raw->i_projid); +} + +static void ext4_inode_projid_set(struct inode *inode, struct ext4_inode *raw, + struct ext4_inode_info *ei, projid_t projid) +{ + raw->i_projid = cpu_to_le32(projid); +} + +int ext4_get_projid(struct inode *inode, kprojid_t *projid) +{ + *projid = EXT4_I(inode)->i_projid; + return 0; +} + struct inode *ext4_iget(struct super_block *sb, unsigned long ino) { struct ext4_iloc iloc; @@ -3885,6 +3903,7 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino) int block; uid_t i_uid; gid_t i_gid; + projid_t i_projid; inode = iget_locked(sb, ino); if (!inode) @@ -3935,12 +3954,15 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino) inode->i_mode = le16_to_cpu(raw_inode->i_mode); i_uid = (uid_t)le16_to_cpu(raw_inode->i_uid_low); i_gid = (gid_t)le16_to_cpu(raw_inode->i_gid_low); + i_projid = ext4_inode_projid_get(inode, raw_inode, ei); + if (!(test_opt(inode->i_sb, NO_UID32))) { i_uid |= le16_to_cpu(raw_inode->i_uid_high) << 16; i_gid |= le16_to_cpu(raw_inode->i_gid_high) << 16; } i_uid_write(inode, i_uid); i_gid_write(inode, i_gid); + ei->i_projid = make_kprojid(&init_user_ns, i_projid);; set_nlink(inode, le16_to_cpu(raw_inode->i_links_count)); ext4_clear_state_flags(ei); /* Only relevant on 32-bit archs */ @@ -4163,6 +4185,7 @@ static int ext4_do_update_inode(handle_t *handle, int need_datasync = 0, set_large_file = 0; uid_t i_uid; gid_t i_gid; + projid_t i_projid; spin_lock(&ei->i_raw_lock); @@ -4175,6 +4198,7 @@ static int ext4_do_update_inode(handle_t *handle, raw_inode->i_mode = cpu_to_le16(inode->i_mode); i_uid = i_uid_read(inode); i_gid = i_gid_read(inode); + i_projid = from_kprojid(&init_user_ns, ei->i_projid); if (!(test_opt(inode->i_sb, NO_UID32))) { raw_inode->i_uid_low = cpu_to_le16(low_16_bits(i_uid)); raw_inode->i_gid_low = cpu_to_le16(low_16_bits(i_gid)); @@ -4204,7 +4228,8 @@ static int ext4_do_update_inode(handle_t *handle, EXT4_INODE_SET_XTIME(i_atime, inode, raw_inode); EXT4_EINODE_SET_XTIME(i_crtime, ei, raw_inode); - if (ext4_inode_blocks_set(handle, raw_inode, ei)) { + err = ext4_inode_blocks_set(handle, raw_inode, ei); + if (err) { spin_unlock(&ei->i_raw_lock); goto out_brelse; } @@ -4255,6 +4280,8 @@ static int ext4_do_update_inode(handle_t *handle, ext4_inode_csum_set(inode, raw_inode, ei); + ext4_inode_projid_set(inode, raw_inode, ei, i_projid); + spin_unlock(&ei->i_raw_lock); BUFFER_TRACE(bh, "call ext4_handle_dirty_metadata"); diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 51705f8..1a9e557 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -2944,6 +2944,11 @@ static int ext4_link(struct dentry *old_dentry, if (inode->i_nlink >= EXT4_LINK_MAX) return -EMLINK; + if ((ext4_test_inode_flag(dir, EXT4_INODE_PROJINHERIT)) && + (!projid_eq(EXT4_I(dir)->i_projid, + EXT4_I(old_dentry->d_inode)->i_projid))) + return -EXDEV; + dquot_initialize(dir); retry: @@ -3186,6 +3191,11 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry, int force_reread; int retval; + if ((ext4_test_inode_flag(new_dir, EXT4_INODE_PROJINHERIT)) && + (!projid_eq(EXT4_I(new_dir)->i_projid, + EXT4_I(old_dentry->d_inode)->i_projid))) + return -EXDEV; + dquot_initialize(old.dir); dquot_initialize(new.dir); diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 2632017..8c3e87b 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1080,6 +1080,7 @@ static const struct dquot_operations ext4_quota_operations = { .write_info = ext4_write_info, .alloc_dquot = dquot_alloc, .destroy_dquot = dquot_destroy, + .get_projid = ext4_get_projid, }; static const struct quotactl_ops ext4_qctl_operations = { -- 1.7.1 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] Adds project ID support for ext4 2014-09-24 14:04 ` [PATCH 2/4] Adds project ID support for ext4 Li Xi @ 2014-09-24 17:11 ` Jan Kara 2014-09-25 1:09 ` Li Xi 0 siblings, 1 reply; 41+ messages in thread From: Jan Kara @ 2014-09-24 17:11 UTC (permalink / raw) To: Li Xi Cc: linux-fsdevel, linux-ext4, linux-api, tytso, adilger, jack, viro, hch, dmonakhov On Wed 24-09-14 22:04:28, Li Xi wrote: > This patch adds a new internal field of ext4 inode to save project > identifier. Also a new flag EXT4_INODE_PROJINHERIT is added for > inheriting project ID from parent directory. Nice. Just a few smaller things below. Firstly, I think your patch forgets to add a check to EXT4_IOC_SETFLAGS ioctl so that EXT4_INODE_PROJINHERIT flag can be set only when EXT4_FEATURE_RO_COMPAT_PROJECT is enabled. > Signed-off-by: Li Xi <lixi@ddn.com> > --- > fs/ext4/ext4.h | 21 +++++++++++++++++---- > fs/ext4/ialloc.c | 6 ++++++ > fs/ext4/inode.c | 29 ++++++++++++++++++++++++++++- > fs/ext4/namei.c | 10 ++++++++++ > fs/ext4/super.c | 1 + > 5 files changed, 62 insertions(+), 5 deletions(-) > ... > diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c > index 5b87fc3..b6b7284 100644 > --- a/fs/ext4/ialloc.c > +++ b/fs/ext4/ialloc.c > @@ -756,6 +756,12 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir, > inode->i_gid = dir->i_gid; > } else > inode_init_owner(inode, dir, mode); > + if (ext4_test_inode_flag(dir, EXT4_INODE_PROJINHERIT)) { > + ei->i_projid = EXT4_I(dir)->i_projid; > + } else { > + ei->i_projid = > + make_kprojid(&init_user_ns, EXT4_DEF_PROJID); > + } > dquot_initialize(inode); > > if (!goal) > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index d5dd7d4..8c641eb 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -3874,6 +3874,24 @@ static inline void ext4_iget_extra_inode(struct inode *inode, > EXT4_I(inode)->i_inline_off = 0; > } > > +static projid_t ext4_inode_projid_get(struct inode *inode, struct ext4_inode *raw, > + struct ext4_inode_info *ei) > +{ > + return (projid_t)le32_to_cpu(raw->i_projid); > +} > + > +static void ext4_inode_projid_set(struct inode *inode, struct ext4_inode *raw, > + struct ext4_inode_info *ei, projid_t projid) > +{ > + raw->i_projid = cpu_to_le32(projid); > +} I don't see a need for these two wrappers. IMHO they just obfuscate the code. Please just write the code directly in appropriate place. > @@ -4204,7 +4228,8 @@ static int ext4_do_update_inode(handle_t *handle, > EXT4_INODE_SET_XTIME(i_atime, inode, raw_inode); > EXT4_EINODE_SET_XTIME(i_crtime, ei, raw_inode); > > - if (ext4_inode_blocks_set(handle, raw_inode, ei)) { > + err = ext4_inode_blocks_set(handle, raw_inode, ei); > + if (err) { > spin_unlock(&ei->i_raw_lock); > goto out_brelse; > } This is an unrelated fix, right? Please send as a separate patch. Thanks! > @@ -4255,6 +4280,8 @@ static int ext4_do_update_inode(handle_t *handle, > > ext4_inode_csum_set(inode, raw_inode, ei); > > + ext4_inode_projid_set(inode, raw_inode, ei, i_projid); > + This is wrong - you have to set project ID before a checksum is computed. Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] Adds project ID support for ext4 2014-09-24 17:11 ` Jan Kara @ 2014-09-25 1:09 ` Li Xi 0 siblings, 0 replies; 41+ messages in thread From: Li Xi @ 2014-09-25 1:09 UTC (permalink / raw) To: Jan Kara Cc: linux-fsdevel, Ext4 Developers List, linux-api, Theodore Ts'o, Andreas Dilger, viro, hch, Dmitry Monakhov On Thu, Sep 25, 2014 at 1:11 AM, Jan Kara <jack@suse.cz> wrote: > On Wed 24-09-14 22:04:28, Li Xi wrote: >> This patch adds a new internal field of ext4 inode to save project >> identifier. Also a new flag EXT4_INODE_PROJINHERIT is added for >> inheriting project ID from parent directory. > Nice. Just a few smaller things below. > > Firstly, I think your patch forgets to add a check to EXT4_IOC_SETFLAGS > ioctl so that EXT4_INODE_PROJINHERIT flag can be set only when > EXT4_FEATURE_RO_COMPAT_PROJECT is enabled. PROJINHERIT is a flag which affects the project ID inheriting. Is there any special reason that this flag can be set/cleared only if EXT4_FEATURE_RO_COMPAT_PROJECT is enabled? We don't check that feature even when set/get project ID now, so I am not so sure that this is consistent. > >> Signed-off-by: Li Xi <lixi@ddn.com> >> --- >> fs/ext4/ext4.h | 21 +++++++++++++++++---- >> fs/ext4/ialloc.c | 6 ++++++ >> fs/ext4/inode.c | 29 ++++++++++++++++++++++++++++- >> fs/ext4/namei.c | 10 ++++++++++ >> fs/ext4/super.c | 1 + >> 5 files changed, 62 insertions(+), 5 deletions(-) >> > ... >> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c >> index 5b87fc3..b6b7284 100644 >> --- a/fs/ext4/ialloc.c >> +++ b/fs/ext4/ialloc.c >> @@ -756,6 +756,12 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir, >> inode->i_gid = dir->i_gid; >> } else >> inode_init_owner(inode, dir, mode); >> + if (ext4_test_inode_flag(dir, EXT4_INODE_PROJINHERIT)) { >> + ei->i_projid = EXT4_I(dir)->i_projid; >> + } else { >> + ei->i_projid = >> + make_kprojid(&init_user_ns, EXT4_DEF_PROJID); >> + } >> dquot_initialize(inode); >> >> if (!goal) >> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c >> index d5dd7d4..8c641eb 100644 >> --- a/fs/ext4/inode.c >> +++ b/fs/ext4/inode.c >> @@ -3874,6 +3874,24 @@ static inline void ext4_iget_extra_inode(struct inode *inode, >> EXT4_I(inode)->i_inline_off = 0; >> } >> >> +static projid_t ext4_inode_projid_get(struct inode *inode, struct ext4_inode *raw, >> + struct ext4_inode_info *ei) >> +{ >> + return (projid_t)le32_to_cpu(raw->i_projid); >> +} >> + >> +static void ext4_inode_projid_set(struct inode *inode, struct ext4_inode *raw, >> + struct ext4_inode_info *ei, projid_t projid) >> +{ >> + raw->i_projid = cpu_to_le32(projid); >> +} > I don't see a need for these two wrappers. IMHO they just obfuscate the > code. Please just write the code directly in appropriate place. Sure. > >> @@ -4204,7 +4228,8 @@ static int ext4_do_update_inode(handle_t *handle, >> EXT4_INODE_SET_XTIME(i_atime, inode, raw_inode); >> EXT4_EINODE_SET_XTIME(i_crtime, ei, raw_inode); >> >> - if (ext4_inode_blocks_set(handle, raw_inode, ei)) { >> + err = ext4_inode_blocks_set(handle, raw_inode, ei); >> + if (err) { >> spin_unlock(&ei->i_raw_lock); >> goto out_brelse; >> } > This is an unrelated fix, right? Please send as a separate patch. Thanks! No problem! > >> @@ -4255,6 +4280,8 @@ static int ext4_do_update_inode(handle_t *handle, >> >> ext4_inode_csum_set(inode, raw_inode, ei); >> >> + ext4_inode_projid_set(inode, raw_inode, ei, i_projid); >> + > This is wrong - you have to set project ID before a checksum is computed. Yeah, sorry for this mistake. Regards, - Li Xi ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 3/4] Adds project quota support for ext4 2014-09-24 14:04 [PATCH 0/4] quota: add project quota support Li Xi 2014-09-24 14:04 ` [PATCH 1/4] Adds general codes to enforces project quota limits Li Xi 2014-09-24 14:04 ` [PATCH 2/4] Adds project ID support for ext4 Li Xi @ 2014-09-24 14:04 ` Li Xi [not found] ` <1411567470-31799-4-git-send-email-lixi-LfVdkaOWEx8@public.gmane.org> 2014-09-24 14:04 ` [PATCH 4/4] Adds ioctl interface support for ext4 project Li Xi 3 siblings, 1 reply; 41+ messages in thread From: Li Xi @ 2014-09-24 14:04 UTC (permalink / raw) To: linux-fsdevel, linux-ext4, linux-api, tytso, adilger, jack, viro, hch, dmonakhov This patch adds mount options for enabling/disabling project quota accounting and enforcement. A new specific inode is also used for project quota accounting. Signed-off-by: Li Xi <lixi@ddn.com> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org> --- fs/ext4/ext4.h | 8 +++- fs/ext4/super.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 118 insertions(+), 13 deletions(-) diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 092f60c..f8be9bf 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -217,6 +217,7 @@ struct ext4_io_submit { #define EXT4_UNDEL_DIR_INO 6 /* Undelete directory inode */ #define EXT4_RESIZE_INO 7 /* Reserved group descriptors inode */ #define EXT4_JOURNAL_INO 8 /* Journal inode */ +#define EXT4_PRJ_QUOTA_INO 9 /* Project quota inode */ /* First non-reserved inode for old ext4 filesystems */ #define EXT4_GOOD_OLD_FIRST_INO 11 @@ -991,6 +992,7 @@ struct ext4_inode_info { #define EXT4_MOUNT_DIOREAD_NOLOCK 0x400000 /* Enable support for dio read nolocking */ #define EXT4_MOUNT_JOURNAL_CHECKSUM 0x800000 /* Journal checksums */ #define EXT4_MOUNT_JOURNAL_ASYNC_COMMIT 0x1000000 /* Journal Async Commit */ +#define EXT4_MOUNT_PRJQUOTA 0x2000000 /* Project quota support */ #define EXT4_MOUNT_DELALLOC 0x8000000 /* Delalloc support */ #define EXT4_MOUNT_DATA_ERR_ABORT 0x10000000 /* Abort on file data write */ #define EXT4_MOUNT_BLOCK_VALIDITY 0x20000000 /* Block validity checking */ @@ -1166,7 +1168,8 @@ struct ext4_super_block { __le32 s_grp_quota_inum; /* inode for tracking group quota */ __le32 s_overhead_clusters; /* overhead blocks/clusters in fs */ __le32 s_backup_bgs[2]; /* groups with sparse_super2 SBs */ - __le32 s_reserved[106]; /* Padding to the end of the block */ + __le32 s_prj_quota_inum; /* inode for tracking project quota */ + __le32 s_reserved[105]; /* Padding to the end of the block */ __le32 s_checksum; /* crc32c(superblock) */ }; @@ -1181,7 +1184,7 @@ struct ext4_super_block { #define EXT4_MF_FS_ABORTED 0x0002 /* Fatal error detected */ /* Number of quota types we support */ -#define EXT4_MAXQUOTAS 2 +#define EXT4_MAXQUOTAS 3 /* * fourth extended-fs super-block data in memory @@ -1372,6 +1375,7 @@ static inline int ext4_valid_inum(struct super_block *sb, unsigned long ino) ino == EXT4_BOOT_LOADER_INO || ino == EXT4_JOURNAL_INO || ino == EXT4_RESIZE_INO || + ino == EXT4_PRJ_QUOTA_INO || (ino >= EXT4_FIRST_INO(sb) && ino <= le32_to_cpu(EXT4_SB(sb)->s_es->s_inodes_count)); } diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 8c3e87b..5cb5055 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1048,8 +1048,8 @@ static int bdev_try_to_free_page(struct super_block *sb, struct page *page, } #ifdef CONFIG_QUOTA -#define QTYPE2NAME(t) ((t) == USRQUOTA ? "user" : "group") -#define QTYPE2MOPT(on, t) ((t) == USRQUOTA?((on)##USRJQUOTA):((on)##GRPJQUOTA)) +static char *quotatypes[] = INITQFNAMES; +#define QTYPE2NAME(t) (quotatypes[t]) static int ext4_write_dquot(struct dquot *dquot); static int ext4_acquire_dquot(struct dquot *dquot); @@ -1160,10 +1160,11 @@ enum { Opt_journal_path, Opt_journal_checksum, Opt_journal_async_commit, Opt_abort, Opt_data_journal, Opt_data_ordered, Opt_data_writeback, Opt_data_err_abort, Opt_data_err_ignore, - Opt_usrjquota, Opt_grpjquota, Opt_offusrjquota, Opt_offgrpjquota, + Opt_usrjquota, Opt_grpjquota, Opt_prjjquota, + Opt_offusrjquota, Opt_offgrpjquota, Opt_offprjjquota, Opt_jqfmt_vfsold, Opt_jqfmt_vfsv0, Opt_jqfmt_vfsv1, Opt_quota, Opt_noquota, Opt_barrier, Opt_nobarrier, Opt_err, - Opt_usrquota, Opt_grpquota, Opt_i_version, + Opt_usrquota, Opt_grpquota, Opt_prjquota, Opt_i_version, Opt_stripe, Opt_delalloc, Opt_nodelalloc, Opt_mblk_io_submit, Opt_nomblk_io_submit, Opt_block_validity, Opt_noblock_validity, Opt_inode_readahead_blks, Opt_journal_ioprio, @@ -1214,6 +1215,8 @@ static const match_table_t tokens = { {Opt_usrjquota, "usrjquota=%s"}, {Opt_offgrpjquota, "grpjquota="}, {Opt_grpjquota, "grpjquota=%s"}, + {Opt_offprjjquota, "prjjquota="}, + {Opt_prjjquota, "prjjquota=%s"}, {Opt_jqfmt_vfsold, "jqfmt=vfsold"}, {Opt_jqfmt_vfsv0, "jqfmt=vfsv0"}, {Opt_jqfmt_vfsv1, "jqfmt=vfsv1"}, @@ -1221,6 +1224,7 @@ static const match_table_t tokens = { {Opt_noquota, "noquota"}, {Opt_quota, "quota"}, {Opt_usrquota, "usrquota"}, + {Opt_prjquota, "prjquota"}, {Opt_barrier, "barrier=%u"}, {Opt_barrier, "barrier"}, {Opt_nobarrier, "nobarrier"}, @@ -1433,6 +1437,8 @@ static const struct mount_opts { MOPT_SET | MOPT_Q}, {Opt_grpquota, EXT4_MOUNT_QUOTA | EXT4_MOUNT_GRPQUOTA, MOPT_SET | MOPT_Q}, + {Opt_prjquota, EXT4_MOUNT_QUOTA | EXT4_MOUNT_PRJQUOTA, + MOPT_SET | MOPT_Q}, {Opt_noquota, (EXT4_MOUNT_QUOTA | EXT4_MOUNT_USRQUOTA | EXT4_MOUNT_GRPQUOTA), MOPT_CLEAR | MOPT_Q}, {Opt_usrjquota, 0, MOPT_Q}, @@ -1461,10 +1467,14 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token, return set_qf_name(sb, USRQUOTA, &args[0]); else if (token == Opt_grpjquota) return set_qf_name(sb, GRPQUOTA, &args[0]); + else if (token == Opt_prjjquota) + return set_qf_name(sb, PRJQUOTA, &args[0]); else if (token == Opt_offusrjquota) return clear_qf_name(sb, USRQUOTA); else if (token == Opt_offgrpjquota) return clear_qf_name(sb, GRPQUOTA); + else if (token == Opt_offprjjquota) + return clear_qf_name(sb, PRJQUOTA); #endif switch (token) { case Opt_noacl: @@ -1690,19 +1700,28 @@ static int parse_options(char *options, struct super_block *sb, } #ifdef CONFIG_QUOTA if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_QUOTA) && - (test_opt(sb, USRQUOTA) || test_opt(sb, GRPQUOTA))) { + (test_opt(sb, USRQUOTA) || + test_opt(sb, GRPQUOTA) || + test_opt(sb, PRJQUOTA))) { ext4_msg(sb, KERN_ERR, "Cannot set quota options when QUOTA " "feature is enabled"); return 0; } - if (sbi->s_qf_names[USRQUOTA] || sbi->s_qf_names[GRPQUOTA]) { + if (sbi->s_qf_names[USRQUOTA] || + sbi->s_qf_names[GRPQUOTA] || + sbi->s_qf_names[PRJQUOTA]) { if (test_opt(sb, USRQUOTA) && sbi->s_qf_names[USRQUOTA]) clear_opt(sb, USRQUOTA); if (test_opt(sb, GRPQUOTA) && sbi->s_qf_names[GRPQUOTA]) clear_opt(sb, GRPQUOTA); - if (test_opt(sb, GRPQUOTA) || test_opt(sb, USRQUOTA)) { + if (test_opt(sb, PRJQUOTA) && sbi->s_qf_names[PRJQUOTA]) + clear_opt(sb, PRJQUOTA); + + if (test_opt(sb, GRPQUOTA) || + test_opt(sb, USRQUOTA) || + test_opt(sb, PRJQUOTA)) { ext4_msg(sb, KERN_ERR, "old and new quota " "format mixing"); return 0; @@ -1763,6 +1782,9 @@ static inline void ext4_show_quota_options(struct seq_file *seq, if (sbi->s_qf_names[GRPQUOTA]) seq_printf(seq, ",grpjquota=%s", sbi->s_qf_names[GRPQUOTA]); + + if (sbi->s_qf_names[PRJQUOTA]) + seq_printf(seq, ",prjjquota=%s", sbi->s_qf_names[PRJQUOTA]); #endif } @@ -2833,6 +2855,13 @@ static int ext4_feature_set_ok(struct super_block *sb, int readonly) "without CONFIG_QUOTA"); return 0; } + if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_PROJECT) && + !readonly) { + ext4_msg(sb, KERN_ERR, + "Filesystem with project quota feature cannot be" + "mounted RDWR without CONFIG_QUOTA"); + return 0; + } #endif /* CONFIG_QUOTA */ return 1; } @@ -5052,6 +5081,46 @@ restore_opts: return err; } +static int ext4_statfs_project(struct super_block *sb, + kprojid_t projid, struct kstatfs *buf) +{ + struct kqid qid; + struct dquot *dquot; + u64 limit; + u64 curblock; + + qid = make_kqid_projid(projid); + dquot = dqget(sb, qid); + if (!dquot) + return -ESRCH; + spin_lock(&dq_data_lock); + + limit = dquot->dq_dqb.dqb_bsoftlimit ? + dquot->dq_dqb.dqb_bsoftlimit : + dquot->dq_dqb.dqb_bhardlimit; + if (limit && buf->f_blocks * buf->f_bsize > limit) { + curblock = dquot->dq_dqb.dqb_curspace / buf->f_bsize; + buf->f_blocks = limit / buf->f_bsize; + buf->f_bfree = buf->f_bavail = + (buf->f_blocks > curblock) ? + (buf->f_blocks - curblock) : 0; + } + + limit = dquot->dq_dqb.dqb_isoftlimit ? + dquot->dq_dqb.dqb_isoftlimit : + dquot->dq_dqb.dqb_ihardlimit; + if (limit && buf->f_files > limit) { + buf->f_files = limit; + buf->f_ffree = + (buf->f_files > dquot->dq_dqb.dqb_curinodes) ? + (buf->f_ffree - dquot->dq_dqb.dqb_curinodes) : 0; + } + + spin_unlock(&dq_data_lock); + dqput(dquot); + return 0; +} + static int ext4_statfs(struct dentry *dentry, struct kstatfs *buf) { struct super_block *sb = dentry->d_sb; @@ -5060,6 +5129,8 @@ static int ext4_statfs(struct dentry *dentry, struct kstatfs *buf) ext4_fsblk_t overhead = 0, resv_blocks; u64 fsid; s64 bfree; + struct inode *inode = dentry->d_inode; + int err = 0; resv_blocks = EXT4_C2B(sbi, atomic64_read(&sbi->s_resv_clusters)); if (!test_opt(sb, MINIX_DF)) @@ -5084,7 +5155,18 @@ static int ext4_statfs(struct dentry *dentry, struct kstatfs *buf) buf->f_fsid.val[0] = fsid & 0xFFFFFFFFUL; buf->f_fsid.val[1] = (fsid >> 32) & 0xFFFFFFFFUL; - return 0; + if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_PROJECT) && + ext4_test_inode_flag(inode, EXT4_INODE_PROJINHERIT) && + sb_has_quota_usage_enabled(sb, PRJQUOTA) && + sb_has_quota_limits_enabled(sb, PRJQUOTA)) { + err = ext4_statfs_project(sb, EXT4_I(inode)->i_projid, buf); + if (err) { + ext4_warning(sb, "Cannot get quota of project %u\n", + from_kprojid(&init_user_ns, + EXT4_I(inode)->i_projid)); + } + } + return err; } /* Helper function for writing quotas on sync - we need to start transaction @@ -5164,7 +5246,9 @@ static int ext4_mark_dquot_dirty(struct dquot *dquot) /* Are we journaling quotas? */ if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_QUOTA) || - sbi->s_qf_names[USRQUOTA] || sbi->s_qf_names[GRPQUOTA]) { + sbi->s_qf_names[USRQUOTA] || + sbi->s_qf_names[GRPQUOTA] || + sbi->s_qf_names[PRJQUOTA]) { dquot_mark_dquot_dirty(dquot); return ext4_write_dquot(dquot); } else { @@ -5248,10 +5332,14 @@ static int ext4_quota_enable(struct super_block *sb, int type, int format_id, struct inode *qf_inode; unsigned long qf_inums[EXT4_MAXQUOTAS] = { le32_to_cpu(EXT4_SB(sb)->s_es->s_usr_quota_inum), - le32_to_cpu(EXT4_SB(sb)->s_es->s_grp_quota_inum) + le32_to_cpu(EXT4_SB(sb)->s_es->s_grp_quota_inum), + le32_to_cpu(EXT4_SB(sb)->s_es->s_prj_quota_inum) }; BUG_ON(!EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_QUOTA)); + BUG_ON(type == PRJQUOTA && + (!EXT4_HAS_RO_COMPAT_FEATURE(sb, + EXT4_FEATURE_RO_COMPAT_PROJECT))); if (!qf_inums[type]) return -EPERM; @@ -5276,11 +5364,16 @@ static int ext4_enable_quotas(struct super_block *sb) int type, err = 0; unsigned long qf_inums[EXT4_MAXQUOTAS] = { le32_to_cpu(EXT4_SB(sb)->s_es->s_usr_quota_inum), - le32_to_cpu(EXT4_SB(sb)->s_es->s_grp_quota_inum) + le32_to_cpu(EXT4_SB(sb)->s_es->s_grp_quota_inum), + le32_to_cpu(EXT4_SB(sb)->s_es->s_prj_quota_inum) }; sb_dqopt(sb)->flags |= DQUOT_QUOTA_SYS_FILE; for (type = 0; type < EXT4_MAXQUOTAS; type++) { + if (type == PRJQUOTA && + (!EXT4_HAS_RO_COMPAT_FEATURE(sb, + EXT4_FEATURE_RO_COMPAT_PROJECT))) + continue; if (qf_inums[type]) { err = ext4_quota_enable(sb, type, QFMT_VFS_V1, DQUOT_USAGE_ENABLED); @@ -5305,6 +5398,10 @@ static int ext4_quota_on_sysfile(struct super_block *sb, int type, if (!EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_QUOTA)) return -EINVAL; + if (type == PRJQUOTA && + (!EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_PROJECT))) + return -EINVAL; + /* * USAGE was enabled at mount time. Only need to enable LIMITS now. */ @@ -5345,6 +5442,10 @@ static int ext4_quota_off_sysfile(struct super_block *sb, int type) if (!EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_QUOTA)) return -EINVAL; + if (type == PRJQUOTA && + (!EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_PROJECT))) + return -EINVAL; + /* Disable only the limits. */ return dquot_disable(sb, type, DQUOT_LIMITS_ENABLED); } -- 1.7.1 ^ permalink raw reply related [flat|nested] 41+ messages in thread
[parent not found: <1411567470-31799-4-git-send-email-lixi-LfVdkaOWEx8@public.gmane.org>]
* Re: [PATCH 3/4] Adds project quota support for ext4 [not found] ` <1411567470-31799-4-git-send-email-lixi-LfVdkaOWEx8@public.gmane.org> @ 2014-09-24 17:31 ` Jan Kara [not found] ` <20140924173123.GH27000-+0h/O2h83AeN3ZZ/Hiejyg@public.gmane.org> 0 siblings, 1 reply; 41+ messages in thread From: Jan Kara @ 2014-09-24 17:31 UTC (permalink / raw) To: Li Xi Cc: linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-ext4-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA, tytso-3s7WtUTddSA, adilger-m1MBpc4rdrD3fQ9qLvQP4Q, jack-AlSwsSmVLrQ, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, hch-wEGCiKHe2LqWVfeAwA7xHQ, dmonakhov-GEFAQzZX7r8dnm+yROfE0A On Wed 24-09-14 22:04:29, Li Xi wrote: > This patch adds mount options for enabling/disabling project quota > accounting and enforcement. A new specific inode is also used for > project quota accounting. The patch looks mostly fine. A few smaller things below. ... > @@ -1433,6 +1437,8 @@ static const struct mount_opts { > MOPT_SET | MOPT_Q}, > {Opt_grpquota, EXT4_MOUNT_QUOTA | EXT4_MOUNT_GRPQUOTA, > MOPT_SET | MOPT_Q}, > + {Opt_prjquota, EXT4_MOUNT_QUOTA | EXT4_MOUNT_PRJQUOTA, > + MOPT_SET | MOPT_Q}, > {Opt_noquota, (EXT4_MOUNT_QUOTA | EXT4_MOUNT_USRQUOTA | > EXT4_MOUNT_GRPQUOTA), MOPT_CLEAR | MOPT_Q}, I think you missed to add EXT4_MOUNT_PRJQUOTA to Opt_noquota... ... > @@ -2833,6 +2855,13 @@ static int ext4_feature_set_ok(struct super_block *sb, int readonly) > "without CONFIG_QUOTA"); > return 0; > } > + if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_PROJECT) && > + !readonly) { > + ext4_msg(sb, KERN_ERR, > + "Filesystem with project quota feature cannot be" > + "mounted RDWR without CONFIG_QUOTA"); > + return 0; > + } Hum, I don't think this is right. EXT4_FEATURE_RO_COMPAT_PROJECT is about maintaining project IDs not about quota. So it seems perfectly OK to have EXT4_FEATURE_RO_COMPAT_PROJECT without CONFIG_QUOTA. ... > @@ -5060,6 +5129,8 @@ static int ext4_statfs(struct dentry *dentry, struct kstatfs *buf) > ext4_fsblk_t overhead = 0, resv_blocks; > u64 fsid; > s64 bfree; > + struct inode *inode = dentry->d_inode; > + int err = 0; > resv_blocks = EXT4_C2B(sbi, atomic64_read(&sbi->s_resv_clusters)); > > if (!test_opt(sb, MINIX_DF)) > @@ -5084,7 +5155,18 @@ static int ext4_statfs(struct dentry *dentry, struct kstatfs *buf) > buf->f_fsid.val[0] = fsid & 0xFFFFFFFFUL; > buf->f_fsid.val[1] = (fsid >> 32) & 0xFFFFFFFFUL; > > - return 0; > + if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_PROJECT) && > + ext4_test_inode_flag(inode, EXT4_INODE_PROJINHERIT) && > + sb_has_quota_usage_enabled(sb, PRJQUOTA) && > + sb_has_quota_limits_enabled(sb, PRJQUOTA)) { If limits are enabled, then usage is enabled. So the check for usage enabled is superfluous... > + err = ext4_statfs_project(sb, EXT4_I(inode)->i_projid, buf); > + if (err) { > + ext4_warning(sb, "Cannot get quota of project %u\n", > + from_kprojid(&init_user_ns, > + EXT4_I(inode)->i_projid)); > + } The warning seems actually bogus - it can happen that project quota enforcement gets turned off after we check sb_has_quota_limits_enabled() but before calling dget() in ext4_statfs_project(). So I would just treat error from ext4_statfs_project() as project quota being disabled... Honza -- Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> SUSE Labs, CR ^ permalink raw reply [flat|nested] 41+ messages in thread
[parent not found: <20140924173123.GH27000-+0h/O2h83AeN3ZZ/Hiejyg@public.gmane.org>]
* Re: [PATCH 3/4] Adds project quota support for ext4 [not found] ` <20140924173123.GH27000-+0h/O2h83AeN3ZZ/Hiejyg@public.gmane.org> @ 2014-09-25 1:28 ` Li Xi [not found] ` <CAPTn0cB8X3DYprX-oKCu8aV5OLLfJJ2rh9YvMY7re69gi1s-LA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 41+ messages in thread From: Li Xi @ 2014-09-25 1:28 UTC (permalink / raw) To: Jan Kara Cc: linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Ext4 Developers List, linux-api-u79uwXL29TY76Z2rM5mHXA, Theodore Ts'o, Andreas Dilger, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, hch-wEGCiKHe2LqWVfeAwA7xHQ, Dmitry Monakhov On Thu, Sep 25, 2014 at 1:31 AM, Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote: > On Wed 24-09-14 22:04:29, Li Xi wrote: >> This patch adds mount options for enabling/disabling project quota >> accounting and enforcement. A new specific inode is also used for >> project quota accounting. > The patch looks mostly fine. A few smaller things below. > > ... >> @@ -1433,6 +1437,8 @@ static const struct mount_opts { >> MOPT_SET | MOPT_Q}, >> {Opt_grpquota, EXT4_MOUNT_QUOTA | EXT4_MOUNT_GRPQUOTA, >> MOPT_SET | MOPT_Q}, >> + {Opt_prjquota, EXT4_MOUNT_QUOTA | EXT4_MOUNT_PRJQUOTA, >> + MOPT_SET | MOPT_Q}, >> {Opt_noquota, (EXT4_MOUNT_QUOTA | EXT4_MOUNT_USRQUOTA | >> EXT4_MOUNT_GRPQUOTA), MOPT_CLEAR | MOPT_Q}, > I think you missed to add EXT4_MOUNT_PRJQUOTA to Opt_noquota... > > ... >> @@ -2833,6 +2855,13 @@ static int ext4_feature_set_ok(struct super_block *sb, int readonly) >> "without CONFIG_QUOTA"); >> return 0; >> } >> + if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_PROJECT) && >> + !readonly) { >> + ext4_msg(sb, KERN_ERR, >> + "Filesystem with project quota feature cannot be" >> + "mounted RDWR without CONFIG_QUOTA"); >> + return 0; >> + } > Hum, I don't think this is right. EXT4_FEATURE_RO_COMPAT_PROJECT is about > maintaining project IDs not about quota. So it seems perfectly OK to have > EXT4_FEATURE_RO_COMPAT_PROJECT without CONFIG_QUOTA. Ah, I see. This might be my main misunderstanding. I thought it is about maintaining both project IDs and quota. And I misunderstood so that I removed all EXT4_FEATURE_RO_COMPAT_PROJECT checking when set/get project ID. If we only use EXT4_FEATURE_RO_COMPAT_PROJECT to protect imcompatibility of project ID, what about the change of struct ext4_super_block? I am still confused. Please advise. Regards, Li Xi ^ permalink raw reply [flat|nested] 41+ messages in thread
[parent not found: <CAPTn0cB8X3DYprX-oKCu8aV5OLLfJJ2rh9YvMY7re69gi1s-LA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 3/4] Adds project quota support for ext4 [not found] ` <CAPTn0cB8X3DYprX-oKCu8aV5OLLfJJ2rh9YvMY7re69gi1s-LA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2014-09-25 11:55 ` Jan Kara 0 siblings, 0 replies; 41+ messages in thread From: Jan Kara @ 2014-09-25 11:55 UTC (permalink / raw) To: Li Xi Cc: Jan Kara, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Ext4 Developers List, linux-api-u79uwXL29TY76Z2rM5mHXA, Theodore Ts'o, Andreas Dilger, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, hch-wEGCiKHe2LqWVfeAwA7xHQ, Dmitry Monakhov On Thu 25-09-14 09:28:24, Li Xi wrote: > On Thu, Sep 25, 2014 at 1:31 AM, Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote: > > On Wed 24-09-14 22:04:29, Li Xi wrote: > >> This patch adds mount options for enabling/disabling project quota > >> accounting and enforcement. A new specific inode is also used for > >> project quota accounting. > > The patch looks mostly fine. A few smaller things below. > > > > ... > >> @@ -1433,6 +1437,8 @@ static const struct mount_opts { > >> MOPT_SET | MOPT_Q}, > >> {Opt_grpquota, EXT4_MOUNT_QUOTA | EXT4_MOUNT_GRPQUOTA, > >> MOPT_SET | MOPT_Q}, > >> + {Opt_prjquota, EXT4_MOUNT_QUOTA | EXT4_MOUNT_PRJQUOTA, > >> + MOPT_SET | MOPT_Q}, > >> {Opt_noquota, (EXT4_MOUNT_QUOTA | EXT4_MOUNT_USRQUOTA | > >> EXT4_MOUNT_GRPQUOTA), MOPT_CLEAR | MOPT_Q}, > > I think you missed to add EXT4_MOUNT_PRJQUOTA to Opt_noquota... > > > > ... > >> @@ -2833,6 +2855,13 @@ static int ext4_feature_set_ok(struct super_block *sb, int readonly) > >> "without CONFIG_QUOTA"); > >> return 0; > >> } > >> + if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_PROJECT) && > >> + !readonly) { > >> + ext4_msg(sb, KERN_ERR, > >> + "Filesystem with project quota feature cannot be" > >> + "mounted RDWR without CONFIG_QUOTA"); > >> + return 0; > >> + } > > Hum, I don't think this is right. EXT4_FEATURE_RO_COMPAT_PROJECT is about > > maintaining project IDs not about quota. So it seems perfectly OK to have > > EXT4_FEATURE_RO_COMPAT_PROJECT without CONFIG_QUOTA. > Ah, I see. This might be my main misunderstanding. I thought it is > about maintaining > both project IDs and quota. And I misunderstood so that I removed all > EXT4_FEATURE_RO_COMPAT_PROJECT checking when set/get project ID. > If we only use EXT4_FEATURE_RO_COMPAT_PROJECT to protect imcompatibility > of project ID, what about the change of struct ext4_super_block? I am > still confused. So my vision is following: EXT4_FEATURE_RO_COMPAT_PROJECT feature controls whether project IDs can be stored in inode. So you don't allow project ID to be set for a superblock without this feature. This is absolutely necessary since otherwise old kernel could happily mount filesystem with project IDs set and would just overwrite them. The same applies to PROJINHERIT flag. Getting of project ID could be easily done without EXT4_FEATURE_RO_COMPAT_PROJECT feature (just return 0) but I think returning EOPNOTSUPP would be better for userspace programs - they can immediatedy distinguish filesystems without project IDs and filesystems where just the file doesn't have any project ID set. Whether project quota is accounted and enforced is a different matter. Without EXT4_FEATURE_RO_COMPAT_QUOTA feature it is userspace which is responsible for properly setting up quotas so you can just use sb_has_quota_loaded() and similar functions. With EXT4_FEATURE_RO_COMPAT_QUOTA, kernel is responsible for setting up quotas and we decide whether we should setup project quotas depending on whether s_prj_quota_inum is set (tools should then make sure s_prj_quota_inum is set only if EXT4_FEATURE_RO_COMPAT_QUOTA and EXT4_FEATURE_RO_COMPAT_PROJECT are set but we can assert that in the kernel just to make sure). Hope this makes things clearer. Honza -- Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> SUSE Labs, CR ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 4/4] Adds ioctl interface support for ext4 project 2014-09-24 14:04 [PATCH 0/4] quota: add project quota support Li Xi ` (2 preceding siblings ...) 2014-09-24 14:04 ` [PATCH 3/4] Adds project quota " Li Xi @ 2014-09-24 14:04 ` Li Xi 2014-09-24 16:25 ` Jan Kara [not found] ` <1411567470-31799-5-git-send-email-lixi-LfVdkaOWEx8@public.gmane.org> 3 siblings, 2 replies; 41+ messages in thread From: Li Xi @ 2014-09-24 14:04 UTC (permalink / raw) To: linux-fsdevel, linux-ext4, linux-api, tytso, adilger, jack, viro, hch, dmonakhov This patch adds ioctl interface for setting/getting project of ext4. Signed-off-by: Li Xi <lixi@ddn.com> --- Documentation/filesystems/ext4.txt | 4 ++ fs/ext4/ext4.h | 2 + fs/ext4/ioctl.c | 85 ++++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 0 deletions(-) diff --git a/Documentation/filesystems/ext4.txt b/Documentation/filesystems/ext4.txt index 919a329..9c98e62 100644 --- a/Documentation/filesystems/ext4.txt +++ b/Documentation/filesystems/ext4.txt @@ -609,6 +609,10 @@ EXT4_IOC_SWAP_BOOT Swap i_blocks and associated attributes The data blocks of the previous boot loader will be associated with the given inode. + EXT4_IOC_GETPROJECT Get project ID associated with inode. + + EXT4_IOC_SETPROJECT Set Project ID associated with inode. + .............................................................................. References diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index f8be9bf..51946fd 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -617,6 +617,8 @@ enum { #define EXT4_IOC_RESIZE_FS _IOW('f', 16, __u64) #define EXT4_IOC_SWAP_BOOT _IO('f', 17) #define EXT4_IOC_PRECACHE_EXTENTS _IO('f', 18) +#define EXT4_IOC_GETPROJECT _IOR('f', 19, long) +#define EXT4_IOC_SETPROJECT _IOW('f', 20, long) #if defined(__KERNEL__) && defined(CONFIG_COMPAT) /* diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c index 0f2252e..93b7ff4 100644 --- a/fs/ext4/ioctl.c +++ b/fs/ext4/ioctl.c @@ -14,6 +14,8 @@ #include <linux/compat.h> #include <linux/mount.h> #include <linux/file.h> +#include <linux/quotaops.h> +#include <linux/quota.h> #include <asm/uaccess.h> #include "ext4_jbd2.h" #include "ext4.h" @@ -611,6 +613,89 @@ resizefs_out: case EXT4_IOC_PRECACHE_EXTENTS: return ext4_ext_precache(inode); + case EXT4_IOC_GETPROJECT: + { + __u32 projid; + + projid = (__u32)from_kprojid(&init_user_ns, + EXT4_I(inode)->i_projid); + return put_user(projid, (__u32 __user *) arg); + } + case EXT4_IOC_SETPROJECT: + { + __u32 projid; + int err; + handle_t *handle; + kprojid_t kprojid; + struct ext4_iloc iloc; + struct ext4_inode *raw_inode; + + struct dquot *transfer_to[EXT4_MAXQUOTAS] = { }; + + /* Make sure caller can change project. */ + if (!capable(CAP_SYS_ADMIN)) + return -EACCES; + + if (get_user(projid, (__u32 __user *) arg)) + return -EFAULT; + + kprojid = make_kprojid(&init_user_ns, (projid_t)projid); + + if (projid_eq(kprojid, EXT4_I(inode)->i_projid)) + return 0; + + err = mnt_want_write_file(filp); + if (err) + return err; + + err = -EPERM; + mutex_lock(&inode->i_mutex); + /* Is it quota file? Do not allow user to mess with it */ + if (IS_NOQUOTA(inode)) + goto project_out; + + dquot_initialize(inode); + + handle = ext4_journal_start(inode, EXT4_HT_QUOTA, + EXT4_QUOTA_INIT_BLOCKS(sb) + + EXT4_QUOTA_DEL_BLOCKS(sb) + 3); + if (IS_ERR(handle)) { + err = PTR_ERR(handle); + goto project_out; + } + + err = ext4_reserve_inode_write(handle, inode, &iloc); + if (err) + goto project_stop; + + raw_inode = ext4_raw_inode(&iloc); + if ((EXT4_INODE_SIZE(inode->i_sb) <= + EXT4_GOOD_OLD_INODE_SIZE) || + (!EXT4_FITS_IN_INODE(raw_inode, ei, i_projid))) { + err = -EFBIG; + goto project_stop; + } + + transfer_to[PRJQUOTA] = dqget(sb, make_kqid_projid(kprojid)); + if (!transfer_to[PRJQUOTA]) + goto project_set; + + err = __dquot_transfer(inode, transfer_to); + dqput(transfer_to[PRJQUOTA]); + if (err) + goto project_stop; + +project_set: + EXT4_I(inode)->i_projid = kprojid; + inode->i_ctime = ext4_current_time(inode); + err = ext4_mark_iloc_dirty(handle, inode, &iloc); +project_stop: + ext4_journal_stop(handle); +project_out: + mutex_unlock(&inode->i_mutex); + mnt_drop_write_file(filp); + return err; + } default: return -ENOTTY; } -- 1.7.1 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project 2014-09-24 14:04 ` [PATCH 4/4] Adds ioctl interface support for ext4 project Li Xi @ 2014-09-24 16:25 ` Jan Kara [not found] ` <1411567470-31799-5-git-send-email-lixi-LfVdkaOWEx8@public.gmane.org> 1 sibling, 0 replies; 41+ messages in thread From: Jan Kara @ 2014-09-24 16:25 UTC (permalink / raw) To: Li Xi Cc: linux-fsdevel, linux-ext4, linux-api, tytso, adilger, jack, viro, hch, dmonakhov, xfs On Wed 24-09-14 22:04:30, Li Xi wrote: > This patch adds ioctl interface for setting/getting project of ext4. The patch looks good to me. I was just wondering whether it won't be useful to add an ioctl() which isn't ext4 specific. We could just extend ->setattr() to allow setting of project ID (most filesystems would just return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call ->setattr from the generic ioctl. That way userspace won't have to care about filesystem type when setting project ID... What do others think? Honza > Signed-off-by: Li Xi <lixi@ddn.com> > --- > Documentation/filesystems/ext4.txt | 4 ++ > fs/ext4/ext4.h | 2 + > fs/ext4/ioctl.c | 85 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 91 insertions(+), 0 deletions(-) > > diff --git a/Documentation/filesystems/ext4.txt b/Documentation/filesystems/ext4.txt > index 919a329..9c98e62 100644 > --- a/Documentation/filesystems/ext4.txt > +++ b/Documentation/filesystems/ext4.txt > @@ -609,6 +609,10 @@ EXT4_IOC_SWAP_BOOT Swap i_blocks and associated attributes > The data blocks of the previous boot loader > will be associated with the given inode. > > + EXT4_IOC_GETPROJECT Get project ID associated with inode. > + > + EXT4_IOC_SETPROJECT Set Project ID associated with inode. > + > .............................................................................. > > References > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index f8be9bf..51946fd 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -617,6 +617,8 @@ enum { > #define EXT4_IOC_RESIZE_FS _IOW('f', 16, __u64) > #define EXT4_IOC_SWAP_BOOT _IO('f', 17) > #define EXT4_IOC_PRECACHE_EXTENTS _IO('f', 18) > +#define EXT4_IOC_GETPROJECT _IOR('f', 19, long) > +#define EXT4_IOC_SETPROJECT _IOW('f', 20, long) > > #if defined(__KERNEL__) && defined(CONFIG_COMPAT) > /* > diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c > index 0f2252e..93b7ff4 100644 > --- a/fs/ext4/ioctl.c > +++ b/fs/ext4/ioctl.c > @@ -14,6 +14,8 @@ > #include <linux/compat.h> > #include <linux/mount.h> > #include <linux/file.h> > +#include <linux/quotaops.h> > +#include <linux/quota.h> > #include <asm/uaccess.h> > #include "ext4_jbd2.h" > #include "ext4.h" > @@ -611,6 +613,89 @@ resizefs_out: > case EXT4_IOC_PRECACHE_EXTENTS: > return ext4_ext_precache(inode); > > + case EXT4_IOC_GETPROJECT: > + { > + __u32 projid; > + > + projid = (__u32)from_kprojid(&init_user_ns, > + EXT4_I(inode)->i_projid); > + return put_user(projid, (__u32 __user *) arg); > + } > + case EXT4_IOC_SETPROJECT: > + { > + __u32 projid; > + int err; > + handle_t *handle; > + kprojid_t kprojid; > + struct ext4_iloc iloc; > + struct ext4_inode *raw_inode; > + > + struct dquot *transfer_to[EXT4_MAXQUOTAS] = { }; > + > + /* Make sure caller can change project. */ > + if (!capable(CAP_SYS_ADMIN)) > + return -EACCES; > + > + if (get_user(projid, (__u32 __user *) arg)) > + return -EFAULT; > + > + kprojid = make_kprojid(&init_user_ns, (projid_t)projid); > + > + if (projid_eq(kprojid, EXT4_I(inode)->i_projid)) > + return 0; > + > + err = mnt_want_write_file(filp); > + if (err) > + return err; > + > + err = -EPERM; > + mutex_lock(&inode->i_mutex); > + /* Is it quota file? Do not allow user to mess with it */ > + if (IS_NOQUOTA(inode)) > + goto project_out; > + > + dquot_initialize(inode); > + > + handle = ext4_journal_start(inode, EXT4_HT_QUOTA, > + EXT4_QUOTA_INIT_BLOCKS(sb) + > + EXT4_QUOTA_DEL_BLOCKS(sb) + 3); > + if (IS_ERR(handle)) { > + err = PTR_ERR(handle); > + goto project_out; > + } > + > + err = ext4_reserve_inode_write(handle, inode, &iloc); > + if (err) > + goto project_stop; > + > + raw_inode = ext4_raw_inode(&iloc); > + if ((EXT4_INODE_SIZE(inode->i_sb) <= > + EXT4_GOOD_OLD_INODE_SIZE) || > + (!EXT4_FITS_IN_INODE(raw_inode, ei, i_projid))) { > + err = -EFBIG; > + goto project_stop; > + } > + > + transfer_to[PRJQUOTA] = dqget(sb, make_kqid_projid(kprojid)); > + if (!transfer_to[PRJQUOTA]) > + goto project_set; > + > + err = __dquot_transfer(inode, transfer_to); > + dqput(transfer_to[PRJQUOTA]); > + if (err) > + goto project_stop; > + > +project_set: > + EXT4_I(inode)->i_projid = kprojid; > + inode->i_ctime = ext4_current_time(inode); > + err = ext4_mark_iloc_dirty(handle, inode, &iloc); > +project_stop: > + ext4_journal_stop(handle); > +project_out: > + mutex_unlock(&inode->i_mutex); > + mnt_drop_write_file(filp); > + return err; > + } > default: > return -ENOTTY; > } > -- > 1.7.1 > -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project @ 2014-09-24 16:25 ` Jan Kara 0 siblings, 0 replies; 41+ messages in thread From: Jan Kara @ 2014-09-24 16:25 UTC (permalink / raw) To: Li Xi Cc: adilger, jack, linux-api, xfs, hch, dmonakhov, viro, linux-fsdevel, tytso, linux-ext4 On Wed 24-09-14 22:04:30, Li Xi wrote: > This patch adds ioctl interface for setting/getting project of ext4. The patch looks good to me. I was just wondering whether it won't be useful to add an ioctl() which isn't ext4 specific. We could just extend ->setattr() to allow setting of project ID (most filesystems would just return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call ->setattr from the generic ioctl. That way userspace won't have to care about filesystem type when setting project ID... What do others think? Honza > Signed-off-by: Li Xi <lixi@ddn.com> > --- > Documentation/filesystems/ext4.txt | 4 ++ > fs/ext4/ext4.h | 2 + > fs/ext4/ioctl.c | 85 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 91 insertions(+), 0 deletions(-) > > diff --git a/Documentation/filesystems/ext4.txt b/Documentation/filesystems/ext4.txt > index 919a329..9c98e62 100644 > --- a/Documentation/filesystems/ext4.txt > +++ b/Documentation/filesystems/ext4.txt > @@ -609,6 +609,10 @@ EXT4_IOC_SWAP_BOOT Swap i_blocks and associated attributes > The data blocks of the previous boot loader > will be associated with the given inode. > > + EXT4_IOC_GETPROJECT Get project ID associated with inode. > + > + EXT4_IOC_SETPROJECT Set Project ID associated with inode. > + > .............................................................................. > > References > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index f8be9bf..51946fd 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -617,6 +617,8 @@ enum { > #define EXT4_IOC_RESIZE_FS _IOW('f', 16, __u64) > #define EXT4_IOC_SWAP_BOOT _IO('f', 17) > #define EXT4_IOC_PRECACHE_EXTENTS _IO('f', 18) > +#define EXT4_IOC_GETPROJECT _IOR('f', 19, long) > +#define EXT4_IOC_SETPROJECT _IOW('f', 20, long) > > #if defined(__KERNEL__) && defined(CONFIG_COMPAT) > /* > diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c > index 0f2252e..93b7ff4 100644 > --- a/fs/ext4/ioctl.c > +++ b/fs/ext4/ioctl.c > @@ -14,6 +14,8 @@ > #include <linux/compat.h> > #include <linux/mount.h> > #include <linux/file.h> > +#include <linux/quotaops.h> > +#include <linux/quota.h> > #include <asm/uaccess.h> > #include "ext4_jbd2.h" > #include "ext4.h" > @@ -611,6 +613,89 @@ resizefs_out: > case EXT4_IOC_PRECACHE_EXTENTS: > return ext4_ext_precache(inode); > > + case EXT4_IOC_GETPROJECT: > + { > + __u32 projid; > + > + projid = (__u32)from_kprojid(&init_user_ns, > + EXT4_I(inode)->i_projid); > + return put_user(projid, (__u32 __user *) arg); > + } > + case EXT4_IOC_SETPROJECT: > + { > + __u32 projid; > + int err; > + handle_t *handle; > + kprojid_t kprojid; > + struct ext4_iloc iloc; > + struct ext4_inode *raw_inode; > + > + struct dquot *transfer_to[EXT4_MAXQUOTAS] = { }; > + > + /* Make sure caller can change project. */ > + if (!capable(CAP_SYS_ADMIN)) > + return -EACCES; > + > + if (get_user(projid, (__u32 __user *) arg)) > + return -EFAULT; > + > + kprojid = make_kprojid(&init_user_ns, (projid_t)projid); > + > + if (projid_eq(kprojid, EXT4_I(inode)->i_projid)) > + return 0; > + > + err = mnt_want_write_file(filp); > + if (err) > + return err; > + > + err = -EPERM; > + mutex_lock(&inode->i_mutex); > + /* Is it quota file? Do not allow user to mess with it */ > + if (IS_NOQUOTA(inode)) > + goto project_out; > + > + dquot_initialize(inode); > + > + handle = ext4_journal_start(inode, EXT4_HT_QUOTA, > + EXT4_QUOTA_INIT_BLOCKS(sb) + > + EXT4_QUOTA_DEL_BLOCKS(sb) + 3); > + if (IS_ERR(handle)) { > + err = PTR_ERR(handle); > + goto project_out; > + } > + > + err = ext4_reserve_inode_write(handle, inode, &iloc); > + if (err) > + goto project_stop; > + > + raw_inode = ext4_raw_inode(&iloc); > + if ((EXT4_INODE_SIZE(inode->i_sb) <= > + EXT4_GOOD_OLD_INODE_SIZE) || > + (!EXT4_FITS_IN_INODE(raw_inode, ei, i_projid))) { > + err = -EFBIG; > + goto project_stop; > + } > + > + transfer_to[PRJQUOTA] = dqget(sb, make_kqid_projid(kprojid)); > + if (!transfer_to[PRJQUOTA]) > + goto project_set; > + > + err = __dquot_transfer(inode, transfer_to); > + dqput(transfer_to[PRJQUOTA]); > + if (err) > + goto project_stop; > + > +project_set: > + EXT4_I(inode)->i_projid = kprojid; > + inode->i_ctime = ext4_current_time(inode); > + err = ext4_mark_iloc_dirty(handle, inode, &iloc); > +project_stop: > + ext4_journal_stop(handle); > +project_out: > + mutex_unlock(&inode->i_mutex); > + mnt_drop_write_file(filp); > + return err; > + } > default: > return -ENOTTY; > } > -- > 1.7.1 > -- Jan Kara <jack@suse.cz> SUSE Labs, CR _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project 2014-09-24 16:25 ` Jan Kara @ 2014-09-24 16:26 ` Christoph Hellwig -1 siblings, 0 replies; 41+ messages in thread From: Christoph Hellwig @ 2014-09-24 16:26 UTC (permalink / raw) To: Jan Kara Cc: Li Xi, linux-fsdevel, linux-ext4, linux-api, tytso, adilger, viro, hch, dmonakhov, xfs On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote: > On Wed 24-09-14 22:04:30, Li Xi wrote: > > This patch adds ioctl interface for setting/getting project of ext4. > The patch looks good to me. I was just wondering whether it won't be > useful to add an ioctl() which isn't ext4 specific. We could just extend > ->setattr() to allow setting of project ID (most filesystems would just > return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call > ->setattr from the generic ioctl. That way userspace won't have to care > about filesystem type when setting project ID... What do others think? Absolutely. In general I also wonder why this patch doesn't implement the full XFS API. Maybe there is a reason it was considered and rejected, but it would be helpful to document why. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project @ 2014-09-24 16:26 ` Christoph Hellwig 0 siblings, 0 replies; 41+ messages in thread From: Christoph Hellwig @ 2014-09-24 16:26 UTC (permalink / raw) To: Jan Kara Cc: adilger, tytso, linux-api, xfs, hch, dmonakhov, viro, Li Xi, linux-fsdevel, linux-ext4 On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote: > On Wed 24-09-14 22:04:30, Li Xi wrote: > > This patch adds ioctl interface for setting/getting project of ext4. > The patch looks good to me. I was just wondering whether it won't be > useful to add an ioctl() which isn't ext4 specific. We could just extend > ->setattr() to allow setting of project ID (most filesystems would just > return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call > ->setattr from the generic ioctl. That way userspace won't have to care > about filesystem type when setting project ID... What do others think? Absolutely. In general I also wonder why this patch doesn't implement the full XFS API. Maybe there is a reason it was considered and rejected, but it would be helpful to document why. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project 2014-09-24 16:26 ` Christoph Hellwig @ 2014-09-24 17:01 ` Jan Kara -1 siblings, 0 replies; 41+ messages in thread From: Jan Kara @ 2014-09-24 17:01 UTC (permalink / raw) To: Christoph Hellwig Cc: Jan Kara, Li Xi, linux-fsdevel, linux-ext4, linux-api, tytso, adilger, viro, dmonakhov, xfs On Wed 24-09-14 09:26:34, Christoph Hellwig wrote: > On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote: > > On Wed 24-09-14 22:04:30, Li Xi wrote: > > > This patch adds ioctl interface for setting/getting project of ext4. > > The patch looks good to me. I was just wondering whether it won't be > > useful to add an ioctl() which isn't ext4 specific. We could just extend > > ->setattr() to allow setting of project ID (most filesystems would just > > return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call > > ->setattr from the generic ioctl. That way userspace won't have to care > > about filesystem type when setting project ID... What do others think? > > Absolutely. In general I also wonder why this patch doesn't implement > the full XFS API. Maybe there is a reason it was considered and > rejected, but it would be helpful to document why. Do you mean full get/setfsxattr API? That basically contains project ID, flags (those that are currently get/set with FS_IOC_GETFLAGS/SETFLAGS), and extent size hint right? That seems workable and it would also make setting of PROJINHERIT flag fs agnostic. Only we would have to create some generic flags namespace and merge into that ext4 flags and have a translation function for the old ext4 flags. Also I'm afraid we may quickly run out of 32 available flags in xflags so we'd need to extend that. But all this seems to be doable. Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project @ 2014-09-24 17:01 ` Jan Kara 0 siblings, 0 replies; 41+ messages in thread From: Jan Kara @ 2014-09-24 17:01 UTC (permalink / raw) To: Christoph Hellwig Cc: adilger, Jan Kara, linux-api, xfs, dmonakhov, viro, Li Xi, linux-fsdevel, tytso, linux-ext4 On Wed 24-09-14 09:26:34, Christoph Hellwig wrote: > On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote: > > On Wed 24-09-14 22:04:30, Li Xi wrote: > > > This patch adds ioctl interface for setting/getting project of ext4. > > The patch looks good to me. I was just wondering whether it won't be > > useful to add an ioctl() which isn't ext4 specific. We could just extend > > ->setattr() to allow setting of project ID (most filesystems would just > > return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call > > ->setattr from the generic ioctl. That way userspace won't have to care > > about filesystem type when setting project ID... What do others think? > > Absolutely. In general I also wonder why this patch doesn't implement > the full XFS API. Maybe there is a reason it was considered and > rejected, but it would be helpful to document why. Do you mean full get/setfsxattr API? That basically contains project ID, flags (those that are currently get/set with FS_IOC_GETFLAGS/SETFLAGS), and extent size hint right? That seems workable and it would also make setting of PROJINHERIT flag fs agnostic. Only we would have to create some generic flags namespace and merge into that ext4 flags and have a translation function for the old ext4 flags. Also I'm afraid we may quickly run out of 32 available flags in xflags so we'd need to extend that. But all this seems to be doable. Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project 2014-09-24 17:01 ` Jan Kara (?) @ 2014-09-25 7:59 ` Dave Chinner 2014-09-25 11:34 ` lixi ` (2 more replies) -1 siblings, 3 replies; 41+ messages in thread From: Dave Chinner @ 2014-09-25 7:59 UTC (permalink / raw) To: Jan Kara Cc: adilger, tytso, linux-api, xfs, Christoph Hellwig, dmonakhov, viro, Li Xi, linux-fsdevel, linux-ext4 On Wed, Sep 24, 2014 at 07:01:05PM +0200, Jan Kara wrote: > On Wed 24-09-14 09:26:34, Christoph Hellwig wrote: > > On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote: > > > On Wed 24-09-14 22:04:30, Li Xi wrote: > > > > This patch adds ioctl interface for setting/getting project of ext4. > > > The patch looks good to me. I was just wondering whether it won't be > > > useful to add an ioctl() which isn't ext4 specific. We could just extend > > > ->setattr() to allow setting of project ID (most filesystems would just > > > return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call > > > ->setattr from the generic ioctl. That way userspace won't have to care > > > about filesystem type when setting project ID... What do others think? > > > > Absolutely. In general I also wonder why this patch doesn't implement > > the full XFS API. Maybe there is a reason it was considered and > > rejected, but it would be helpful to document why. > Do you mean full get/setfsxattr API? That's a good start. The bigger issue in my mind is that we already have a fully featured quota API that supports project quotas and userspace tools available that manipulate it. xfstests already uses those tools and API for testing project quotas. This whole patchset reinvents all the quota APIs, and will require adding support in userspace, and hence require re-inventing all the test infrastructure we already have because it won't be compatible with the existing project quota test code. > That basically contains project ID, > flags (those that are currently get/set with FS_IOC_GETFLAGS/SETFLAGS), and > extent size hint right? It's a different set of flag definitions. We translate the interface XFS_XFLAG_* values to/from the inode on-disk XFS_DIFLAG_* inode values, just like we translate the VFS FS_*_FL flags that get passed through the FS_IOC_GETFLAGS/SETFLAGS ioctl. > That seems workable and it would also make setting > of PROJINHERIT flag fs agnostic. Only we would have to create some generic > flags namespace and merge into that ext4 flags and have a translation > function for the old ext4 flags. The XFS_XFLAGS_* are already filesystem agnostic - they are flags that are only used for interfacing with userspace and hence only exist at the ioctl copy in/out layer..... > Also I'm afraid we may quickly run out of > 32 available flags in xflags so we'd need to extend that. But all this > seems to be doable. The struct fsxattr was designed to be extensible - it has unused padding and enough space in the flags field to allow us to conditionally use that padding.... Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project 2014-09-25 7:59 ` Dave Chinner @ 2014-09-25 11:34 ` lixi 2014-09-25 13:41 ` Theodore Ts'o 2014-09-25 13:52 ` Jan Kara 2 siblings, 0 replies; 41+ messages in thread From: lixi @ 2014-09-25 11:34 UTC (permalink / raw) To: Dave Chinner Cc: Jan Kara, Christoph Hellwig, Andreas Dilger, linux-api-u79uwXL29TY76Z2rM5mHXA, xfs-VZNHf3L845pBDgjK7y7TUQ, Dmitry Monakhov, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Theodore Ts'o, Ext4 Developers List 在 2014年9月25日,下午3:59,Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> 写道: > On Wed, Sep 24, 2014 at 07:01:05PM +0200, Jan Kara wrote: >> On Wed 24-09-14 09:26:34, Christoph Hellwig wrote: >>> On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote: >>>> On Wed 24-09-14 22:04:30, Li Xi wrote: >>>>> This patch adds ioctl interface for setting/getting project of ext4. >>>> The patch looks good to me. I was just wondering whether it won't be >>>> useful to add an ioctl() which isn't ext4 specific. We could just extend >>>> ->setattr() to allow setting of project ID (most filesystems would just >>>> return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call >>>> ->setattr from the generic ioctl. That way userspace won't have to care >>>> about filesystem type when setting project ID... What do others think? >>> >>> Absolutely. In general I also wonder why this patch doesn't implement >>> the full XFS API. Maybe there is a reason it was considered and >>> rejected, but it would be helpful to document why. >> Do you mean full get/setfsxattr API? > > That's a good start. > > The bigger issue in my mind is that we already have a fully featured > quota API that supports project quotas and userspace tools available > that manipulate it. xfstests already uses those tools and API > for testing project quotas. > > This whole patchset reinvents all the quota APIs, and will require > adding support in userspace, and hence require re-inventing all the > test infrastructure we already have because it won't be compatible > with the existing project quota test code. > >> That basically contains project ID, >> flags (those that are currently get/set with FS_IOC_GETFLAGS/SETFLAGS), and >> extent size hint right? > > It's a different set of flag definitions. We translate the interface > XFS_XFLAG_* values to/from the inode on-disk XFS_DIFLAG_* inode values, just > like we translate the VFS FS_*_FL flags that get passed through the > FS_IOC_GETFLAGS/SETFLAGS ioctl. > >> That seems workable and it would also make setting >> of PROJINHERIT flag fs agnostic. Only we would have to create some generic >> flags namespace and merge into that ext4 flags and have a translation >> function for the old ext4 flags. > > The XFS_XFLAGS_* are already filesystem agnostic - they are flags > that are only used for interfacing with userspace and hence only > exist at the ioctl copy in/out layer..... > >> Also I'm afraid we may quickly run out of >> 32 available flags in xflags so we'd need to extend that. But all this >> seems to be doable. > > The struct fsxattr was designed to be extensible - it has unused > padding and enough space in the flags field to allow us to > conditionally use that padding…. Hi Dave, I was mostly working on the semantics of inherit flag on these patches. And I didn’t realized that the interface differences would bother you so much. Sorry for that. I agree that we should choose a good interface. It would be good that it is general so that it works well for all file systems. I agree that adding an ext4 specific ioctl() is far from the best choice. I am willing to change it to any general interface. A general ioctl() sounds good to me. Extend setattr() and getattr() for project ID sounds even better, since project ID looks like UID/GID. And general xattr name is another choice. But it is might be a little bit confusing if we use xattr actually, since we are not saving project ID as extended attribute on Ext4. Any choice is fine with me, as long as the implementation won't introduce nasty codes or inconsistent design. However, the problem is, I do not quite understand why we should keep the interface exactly the same with XFS. It would be good if we can. But as far as I can see, it seems hard. XFS uses a lot interfaces which are not so standard and used by other file systems. For example, struct fsxattr is not used by other file systems at all except XFS. I am not sure why we should introduce this into Ext4 if there are a lot of other better ways. I would be happy to change to XFS interfaces, if it is general. However, I don’t think it is general enough. I know xfstest is using the existing project quota interfaces of XFS. And maybe there are some applications which are using them too. But keeping the interfaces exactly the same with XFS would cost so much effort that I’d like to get enough reasons before start working on it. Is it really necessary? I am not so sure. It is so easy to change user space applications comparing to changing a weird interfaces. For example, I think it won’t cost even more than a day to add xfstest support for new Ext4 project quota. And since project quota is far from a widely used feature, I don’t think there is much compatibility problems for existing applications. And If the new project interface are general enough, there won’t be any compatibility problems for new applications at all. I might missed something important. So please let me know if you have any advices. Regards, - Li Xi-- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project @ 2014-09-25 11:34 ` lixi 0 siblings, 0 replies; 41+ messages in thread From: lixi @ 2014-09-25 11:34 UTC (permalink / raw) To: Dave Chinner Cc: Andreas Dilger, Jan Kara, linux-api, xfs, Christoph Hellwig, Dmitry Monakhov, viro, linux-fsdevel, Theodore Ts'o, Ext4 Developers List 在 2014年9月25日,下午3:59,Dave Chinner <david@fromorbit.com> 写道: > On Wed, Sep 24, 2014 at 07:01:05PM +0200, Jan Kara wrote: >> On Wed 24-09-14 09:26:34, Christoph Hellwig wrote: >>> On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote: >>>> On Wed 24-09-14 22:04:30, Li Xi wrote: >>>>> This patch adds ioctl interface for setting/getting project of ext4. >>>> The patch looks good to me. I was just wondering whether it won't be >>>> useful to add an ioctl() which isn't ext4 specific. We could just extend >>>> ->setattr() to allow setting of project ID (most filesystems would just >>>> return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call >>>> ->setattr from the generic ioctl. That way userspace won't have to care >>>> about filesystem type when setting project ID... What do others think? >>> >>> Absolutely. In general I also wonder why this patch doesn't implement >>> the full XFS API. Maybe there is a reason it was considered and >>> rejected, but it would be helpful to document why. >> Do you mean full get/setfsxattr API? > > That's a good start. > > The bigger issue in my mind is that we already have a fully featured > quota API that supports project quotas and userspace tools available > that manipulate it. xfstests already uses those tools and API > for testing project quotas. > > This whole patchset reinvents all the quota APIs, and will require > adding support in userspace, and hence require re-inventing all the > test infrastructure we already have because it won't be compatible > with the existing project quota test code. > >> That basically contains project ID, >> flags (those that are currently get/set with FS_IOC_GETFLAGS/SETFLAGS), and >> extent size hint right? > > It's a different set of flag definitions. We translate the interface > XFS_XFLAG_* values to/from the inode on-disk XFS_DIFLAG_* inode values, just > like we translate the VFS FS_*_FL flags that get passed through the > FS_IOC_GETFLAGS/SETFLAGS ioctl. > >> That seems workable and it would also make setting >> of PROJINHERIT flag fs agnostic. Only we would have to create some generic >> flags namespace and merge into that ext4 flags and have a translation >> function for the old ext4 flags. > > The XFS_XFLAGS_* are already filesystem agnostic - they are flags > that are only used for interfacing with userspace and hence only > exist at the ioctl copy in/out layer..... > >> Also I'm afraid we may quickly run out of >> 32 available flags in xflags so we'd need to extend that. But all this >> seems to be doable. > > The struct fsxattr was designed to be extensible - it has unused > padding and enough space in the flags field to allow us to > conditionally use that padding…. Hi Dave, I was mostly working on the semantics of inherit flag on these patches. And I didn’t realized that the interface differences would bother you so much. Sorry for that. I agree that we should choose a good interface. It would be good that it is general so that it works well for all file systems. I agree that adding an ext4 specific ioctl() is far from the best choice. I am willing to change it to any general interface. A general ioctl() sounds good to me. Extend setattr() and getattr() for project ID sounds even better, since project ID looks like UID/GID. And general xattr name is another choice. But it is might be a little bit confusing if we use xattr actually, since we are not saving project ID as extended attribute on Ext4. Any choice is fine with me, as long as the implementation won't introduce nasty codes or inconsistent design. However, the problem is, I do not quite understand why we should keep the interface exactly the same with XFS. It would be good if we can. But as far as I can see, it seems hard. XFS uses a lot interfaces which are not so standard and used by other file systems. For example, struct fsxattr is not used by other file systems at all except XFS. I am not sure why we should introduce this into Ext4 if there are a lot of other better ways. I would be happy to change to XFS interfaces, if it is general. However, I don’t think it is general enough. I know xfstest is using the existing project quota interfaces of XFS. And maybe there are some applications which are using them too. But keeping the interfaces exactly the same with XFS would cost so much effort that I’d like to get enough reasons before start working on it. Is it really necessary? I am not so sure. It is so easy to change user space applications comparing to changing a weird interfaces. For example, I think it won’t cost even more than a day to add xfstest support for new Ext4 project quota. And since project quota is far from a widely used feature, I don’t think there is much compatibility problems for existing applications. And If the new project interface are general enough, there won’t be any compatibility problems for new applications at all. I might missed something important. So please let me know if you have any advices. Regards, - Li Xi _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 41+ messages in thread
[parent not found: <C31A739F-4502-4B40-9AE3-F2FE49291657-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project 2014-09-25 11:34 ` lixi @ 2014-09-26 0:10 ` Dave Chinner -1 siblings, 0 replies; 41+ messages in thread From: Dave Chinner @ 2014-09-26 0:10 UTC (permalink / raw) To: lixi Cc: Jan Kara, Christoph Hellwig, Andreas Dilger, linux-api-u79uwXL29TY76Z2rM5mHXA, xfs-VZNHf3L845pBDgjK7y7TUQ, Dmitry Monakhov, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Theodore Ts'o, Ext4 Developers List On Thu, Sep 25, 2014 at 07:34:38PM +0800, lixi wrote: > Hi Dave, > > I was mostly working on the semantics of inherit flag on these patches. And > I didn’t realized that the interface differences would bother you so much. Sorry > for that. It's not the differences that bother me - it's the fact that I was repeatedly ignored until someone else raised the same issue.... > I agree that we should choose a good interface. It would be good that it is > general so that it works well for all file systems. I agree that adding an > ext4 specific ioctl() is far from the best choice. I am willing to change it to > any general interface. A general ioctl() sounds good to me. Extend setattr() > and getattr() for project ID sounds even better, since project ID looks like > UID/GID. Ah, no, project ID is not a uid/gid. It's a completely independent construct. [ Which brings me to, once again, the issue of being ignored during reviews: project IDs should not be mapped by user namespaces, nor be accessible from anything other than the init namespace. In XFS we've turned off access to project IDs within namespace containers because they are used for container space management (i.e. by the init namespace) to manage the amount of filesystem space a container or set of containers can use. We do not want project IDs to be manipulated from within such containers, and therefore have to prevent access to them from within user namespaces. ] > And general xattr name is another choice. But it is might be a little > bit confusing if we use xattr actually, since we are not saving project ID as > extended attribute on Ext4. Any choice is fine with me, as long as the > implementation won't introduce nasty codes or inconsistent design. We can easily create another ioctl name if we have to. It just needs sto be defined to the same value as the XFS ioctl names currently are. We've done this before when making ioctls that originated in XFS generic (e.g. with freeze/thaw ioctls).... > However, the problem is, I do not quite understand why we should keep > the interface exactly the same with XFS. It would be good if we can. But > as far as I can see, it seems hard. XFS uses a lot interfaces which are > not so standard and used by other file systems. For example, struct > fsxattr is not used by other file systems at all except XFS. Moving a existing structure definitions to a different header file is too hard? > I am not sure why we should introduce this into Ext4 if there are > a lot of other better ways. I would be happy to change to XFS > interfaces, if it is general. However, I don’t think it is > general enough. How is it not general enough? Examples, please, not handwaving: which bit of the quota interface can't ext4 use because it's XFS specific? We already have a perfectly functional interface and a large body of code that implements and tests it. You're saying "oh, it's too much work for me to implement an existing interface" and ignoring the fact that not implementing the existing interface forces a huge amount of downstream work. e.g. - we need completely new test infrastructure to replicate existing tests. - we need new tests to ensure the different APIs and utilities provide the same functionality, and that the work identically. - administrators are going to have to learn how ext4 is different to what they already know and understand. - administrators that has tools written to manage project quotas is going to have to rewrite them to support ext4. It's an entirely selfish argument that ignores what already existing out in userspace. i.e. you're saying that existing downstream users of project quotas simple don't matter to you... > I know xfstest is using the existing project quota interfaces of XFS. And > maybe there are some applications which are using them too. But > keeping the interfaces exactly the same with XFS would cost so much > effort that I’d like to get enough reasons before start working on it. Is it > really necessary? I am not so sure. You have to have a stronger argument than that to justify creating a new incompatible user interface. The XFS interfaces have been available for more than 10 years and support all the functionality ext4 requires. If it was any other userspace interface (e.g. syscalls) or any filesystem other than ext4 there would be people from all over telling you "use the existing interfaces!" and you'd need very strong reasons for creating a new one. i.e. you need to demonstrate that the existing interfaces are inadequate for the intended purpose of the new functionality. That's clearly not the case here so why should we allow you to create an incompatible userspace API rather than use the existing, fully functional API? > It is so easy to change user space applications comparing to > changing a weird interfaces. The existing generic quota tools (i.e quotactl, repquota, etc) already implement the XFS quota API to be able to query XFS filesystems. There's no "changing to wierd interfaces" necessary for userspace; it's already all there. Hence any work you do to add project quota awareness to those generic userspace tools will need to add the support to the XFS queries anyway. IOWs, you're not making it any easier for yourself in userspace by creating a new API for ext4 - it just doubles the amount of work you have to in userspace to make existing tools project quota aware. > For > example, I think it won’t cost even more than a day to add xfstest > support for new Ext4 project quota. A day of whose time? Ever thought about how much time it will take reviewers to look at your tests and iterate over them to get it all right? If you're introducing new userspace infrastructure that xfstests will need to depend on and test for, then it's a lot more than just writing new tests. Indeed, I'm likely to want new project quota tests to be generic (i.e. works and passes on any filesystem that supports project quotas) with the introduction of ext4 project quota support. It's the same functionality and so it should work the same just like user and group quotas do across all filesystems. > And since project quota is far from > a widely used feature, I don't think you realise quite how widespread it's use is on XFS. > I don’t think there is much compatibility problems > for existing applications. And If the new project interface are general > enough, there won’t be any compatibility problems for new applications > at all. Again, you are ignoring the compatibility problems with existing applications that are project quota aware. For them you are *creating new compatibility problems* by implementing a new interface. i.e. Existing applications will not work on ext4, and new applications written to work on ext4 won't work on XFS. That's the crux of the issue - we have existing applications using the existing interface and so introducing a new interface introduces compatibility problems. You can't just wave this problem away because you don't think the existing interface matters. "It's easier for me to create a new interface" is not a valid reason for creating a new interface.... Cheers, Dave. -- Dave Chinner david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project @ 2014-09-26 0:10 ` Dave Chinner 0 siblings, 0 replies; 41+ messages in thread From: Dave Chinner @ 2014-09-26 0:10 UTC (permalink / raw) To: lixi Cc: Andreas Dilger, Jan Kara, linux-api, xfs, Christoph Hellwig, Dmitry Monakhov, viro, linux-fsdevel, Theodore Ts'o, Ext4 Developers List On Thu, Sep 25, 2014 at 07:34:38PM +0800, lixi wrote: > Hi Dave, > > I was mostly working on the semantics of inherit flag on these patches. And > I didn’t realized that the interface differences would bother you so much. Sorry > for that. It's not the differences that bother me - it's the fact that I was repeatedly ignored until someone else raised the same issue.... > I agree that we should choose a good interface. It would be good that it is > general so that it works well for all file systems. I agree that adding an > ext4 specific ioctl() is far from the best choice. I am willing to change it to > any general interface. A general ioctl() sounds good to me. Extend setattr() > and getattr() for project ID sounds even better, since project ID looks like > UID/GID. Ah, no, project ID is not a uid/gid. It's a completely independent construct. [ Which brings me to, once again, the issue of being ignored during reviews: project IDs should not be mapped by user namespaces, nor be accessible from anything other than the init namespace. In XFS we've turned off access to project IDs within namespace containers because they are used for container space management (i.e. by the init namespace) to manage the amount of filesystem space a container or set of containers can use. We do not want project IDs to be manipulated from within such containers, and therefore have to prevent access to them from within user namespaces. ] > And general xattr name is another choice. But it is might be a little > bit confusing if we use xattr actually, since we are not saving project ID as > extended attribute on Ext4. Any choice is fine with me, as long as the > implementation won't introduce nasty codes or inconsistent design. We can easily create another ioctl name if we have to. It just needs sto be defined to the same value as the XFS ioctl names currently are. We've done this before when making ioctls that originated in XFS generic (e.g. with freeze/thaw ioctls).... > However, the problem is, I do not quite understand why we should keep > the interface exactly the same with XFS. It would be good if we can. But > as far as I can see, it seems hard. XFS uses a lot interfaces which are > not so standard and used by other file systems. For example, struct > fsxattr is not used by other file systems at all except XFS. Moving a existing structure definitions to a different header file is too hard? > I am not sure why we should introduce this into Ext4 if there are > a lot of other better ways. I would be happy to change to XFS > interfaces, if it is general. However, I don’t think it is > general enough. How is it not general enough? Examples, please, not handwaving: which bit of the quota interface can't ext4 use because it's XFS specific? We already have a perfectly functional interface and a large body of code that implements and tests it. You're saying "oh, it's too much work for me to implement an existing interface" and ignoring the fact that not implementing the existing interface forces a huge amount of downstream work. e.g. - we need completely new test infrastructure to replicate existing tests. - we need new tests to ensure the different APIs and utilities provide the same functionality, and that the work identically. - administrators are going to have to learn how ext4 is different to what they already know and understand. - administrators that has tools written to manage project quotas is going to have to rewrite them to support ext4. It's an entirely selfish argument that ignores what already existing out in userspace. i.e. you're saying that existing downstream users of project quotas simple don't matter to you... > I know xfstest is using the existing project quota interfaces of XFS. And > maybe there are some applications which are using them too. But > keeping the interfaces exactly the same with XFS would cost so much > effort that I’d like to get enough reasons before start working on it. Is it > really necessary? I am not so sure. You have to have a stronger argument than that to justify creating a new incompatible user interface. The XFS interfaces have been available for more than 10 years and support all the functionality ext4 requires. If it was any other userspace interface (e.g. syscalls) or any filesystem other than ext4 there would be people from all over telling you "use the existing interfaces!" and you'd need very strong reasons for creating a new one. i.e. you need to demonstrate that the existing interfaces are inadequate for the intended purpose of the new functionality. That's clearly not the case here so why should we allow you to create an incompatible userspace API rather than use the existing, fully functional API? > It is so easy to change user space applications comparing to > changing a weird interfaces. The existing generic quota tools (i.e quotactl, repquota, etc) already implement the XFS quota API to be able to query XFS filesystems. There's no "changing to wierd interfaces" necessary for userspace; it's already all there. Hence any work you do to add project quota awareness to those generic userspace tools will need to add the support to the XFS queries anyway. IOWs, you're not making it any easier for yourself in userspace by creating a new API for ext4 - it just doubles the amount of work you have to in userspace to make existing tools project quota aware. > For > example, I think it won’t cost even more than a day to add xfstest > support for new Ext4 project quota. A day of whose time? Ever thought about how much time it will take reviewers to look at your tests and iterate over them to get it all right? If you're introducing new userspace infrastructure that xfstests will need to depend on and test for, then it's a lot more than just writing new tests. Indeed, I'm likely to want new project quota tests to be generic (i.e. works and passes on any filesystem that supports project quotas) with the introduction of ext4 project quota support. It's the same functionality and so it should work the same just like user and group quotas do across all filesystems. > And since project quota is far from > a widely used feature, I don't think you realise quite how widespread it's use is on XFS. > I don’t think there is much compatibility problems > for existing applications. And If the new project interface are general > enough, there won’t be any compatibility problems for new applications > at all. Again, you are ignoring the compatibility problems with existing applications that are project quota aware. For them you are *creating new compatibility problems* by implementing a new interface. i.e. Existing applications will not work on ext4, and new applications written to work on ext4 won't work on XFS. That's the crux of the issue - we have existing applications using the existing interface and so introducing a new interface introduces compatibility problems. You can't just wave this problem away because you don't think the existing interface matters. "It's easier for me to create a new interface" is not a valid reason for creating a new interface.... Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project 2014-09-26 0:10 ` Dave Chinner @ 2014-09-26 2:45 ` Li Xi -1 siblings, 0 replies; 41+ messages in thread From: Li Xi @ 2014-09-26 2:45 UTC (permalink / raw) To: Dave Chinner Cc: Jan Kara, Christoph Hellwig, Andreas Dilger, linux-api-u79uwXL29TY76Z2rM5mHXA, xfs-VZNHf3L845pBDgjK7y7TUQ, Dmitry Monakhov, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Theodore Ts'o, Ext4 Developers List On Fri, Sep 26, 2014 at 8:10 AM, Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote: > On Thu, Sep 25, 2014 at 07:34:38PM +0800, lixi wrote: >> Hi Dave, >> >> I was mostly working on the semantics of inherit flag on these patches. And >> I didn’t realized that the interface differences would bother you so much. Sorry >> for that. > > It's not the differences that bother me - it's the fact that I was > repeatedly ignored until someone else raised the same issue.... Sorry about that. Since we are implementing a feature which exsits in XFS for years, your opions are surely very important. They are not ignored. But I think more discussion is necessary before an common agreement is reached. And I am willing to change the interfaces to whatever all people agree on. > >> I agree that we should choose a good interface. It would be good that it is >> general so that it works well for all file systems. I agree that adding an >> ext4 specific ioctl() is far from the best choice. I am willing to change it to >> any general interface. A general ioctl() sounds good to me. Extend setattr() >> and getattr() for project ID sounds even better, since project ID looks like >> UID/GID. > > Ah, no, project ID is not a uid/gid. It's a completely independent > construct. > > [ Which brings me to, once again, the issue of being ignored during > reviews: project IDs should not be mapped by user namespaces, nor be > accessible from anything other than the init namespace. > > In XFS we've turned off access to project IDs within namespace > containers because they are used for container space management > (i.e. by the init namespace) to manage the amount of filesystem > space a container or set of containers can use. We do not want > project IDs to be manipulated from within such containers, and > therefore have to prevent access to them from within user > namespaces. ] OK. That is another semantics which needs further discussion whether we need to implement for Ext4. Container space management is one of the project quota use case. I am not sure how widely used it is, but I like features with less limits. > >> And general xattr name is another choice. But it is might be a little >> bit confusing if we use xattr actually, since we are not saving project ID as >> extended attribute on Ext4. Any choice is fine with me, as long as the >> implementation won't introduce nasty codes or inconsistent design. > > We can easily create another ioctl name if we have to. It just needs > sto be defined to the same value as the XFS ioctl names currently > are. We've done this before when making ioctls that originated in > XFS generic (e.g. with freeze/thaw ioctls).... OK. I am fine with general ioctl to set/get project ID. > >> However, the problem is, I do not quite understand why we should keep >> the interface exactly the same with XFS. It would be good if we can. But >> as far as I can see, it seems hard. XFS uses a lot interfaces which are >> not so standard and used by other file systems. For example, struct >> fsxattr is not used by other file systems at all except XFS. > > Moving a existing structure definitions to a different header file > is too hard? I don't think difficulty is a big problem. I'm wondering whether it is necessary. > >> I am not sure why we should introduce this into Ext4 if there are >> a lot of other better ways. I would be happy to change to XFS >> interfaces, if it is general. However, I don’t think it is >> general enough. > > How is it not general enough? Examples, please, not handwaving: > which bit of the quota interface can't ext4 use because it's XFS > specific? Well, I am not so familar with XFS, so I might be all wrong about XFS. But honestly, a lot of quota interfaces of XFS seems not so standard. For example, all the Q_X* flags used in do_quotactl(). Maybe there are really good reasons why they are there. I don't know. And XFS does not use general codes under fs/quota. That is a big difference with Ext4. > > We already have a perfectly functional interface and a large body of > code that implements and tests it. You're saying "oh, it's too much > work for me to implement an existing interface" and ignoring the > fact that not implementing the existing interface forces a huge > amount of downstream work. e.g. > > - we need completely new test infrastructure to replicate > existing tests. > - we need new tests to ensure the different APIs and > utilities provide the same functionality, and that the > work identically. > - administrators are going to have to learn how ext4 is > different to what they already know and understand. > - administrators that has tools written to manage project > quotas is going to have to rewrite them to support ext4. > > It's an entirely selfish argument that ignores what already existing > out in userspace. i.e. you're saying that existing downstream users of > project quotas simple don't matter to you... > >> I know xfstest is using the existing project quota interfaces of XFS. And >> maybe there are some applications which are using them too. But >> keeping the interfaces exactly the same with XFS would cost so much >> effort that I’d like to get enough reasons before start working on it. Is it >> really necessary? I am not so sure. > > You have to have a stronger argument than that to justify creating a > new incompatible user interface. The XFS interfaces have been > available for more than 10 years and support all the functionality > ext4 requires. If it was any other userspace interface (e.g. > syscalls) or any filesystem other than ext4 there would be people > from all over telling you "use the existing interfaces!" and you'd > need very strong reasons for creating a new one. > > i.e. you need to demonstrate that the existing interfaces are > inadequate for the intended purpose of the new functionality. That's > clearly not the case here so why should we allow you to create an > incompatible userspace API rather than use the existing, fully > functional API? > >> It is so easy to change user space applications comparing to >> changing a weird interfaces. > > The existing generic quota tools (i.e quotactl, repquota, etc) > already implement the XFS quota API to be able to query XFS > filesystems. There's no "changing to wierd interfaces" necessary > for userspace; it's already all there. Hence any work you do to add > project quota awareness to those generic userspace tools will need to > add the support to the XFS queries anyway. > > IOWs, you're not making it any easier for yourself in userspace by > creating a new API for ext4 - it just doubles the amount of work you > have to in userspace to make existing tools project quota aware. I am afraid I have to make existing tools project quota aware. And actually I've done most of the work. I've updated e2fsprogs and quota-tools to support project quota. Unfortunately these updates are inevitable anyway. As Jan Kara said, we can't force system admins to change from quota-tool command to xfs quota tools. Thus, I add '-P $PROJECT' arguments to all the commands. And based on those tools, I made a script which is less than 1K lines for regression test. It is working pretty well. I don't see a good reason why it is necessary to change everything to XFS way. > >> For >> example, I think it won’t cost even more than a day to add xfstest >> support for new Ext4 project quota. > > A day of whose time? I am always willing to help if you agree. :) > > Ever thought about how much time it will take reviewers to look at > your tests and iterate over them to get it all right? If you're > introducing new userspace infrastructure that xfstests will need to > depend on and test for, then it's a lot more than just writing new > tests. > > Indeed, I'm likely to want new project quota tests to be generic > (i.e. works and passes on any filesystem that supports project > quotas) with the introduction of ext4 project quota support. It's > the same functionality and so it should work the same just like user > and group quotas do across all filesystems. > >> And since project quota is far from >> a widely used feature, > > I don't think you realise quite how widespread it's use is on XFS. > >> I don’t think there is much compatibility problems >> for existing applications. And If the new project interface are general >> enough, there won’t be any compatibility problems for new applications >> at all. > > Again, you are ignoring the compatibility problems with existing > applications that are project quota aware. For them you are > *creating new compatibility problems* by implementing a new > interface. i.e. Existing applications will not work on ext4, and > new applications written to work on ext4 won't work on XFS. > > That's the crux of the issue - we have existing applications using > the existing interface and so introducing a new interface introduces > compatibility problems. You can't just wave this problem away > because you don't think the existing interface matters. > > "It's easier for me to create a new interface" is not a valid reason > for creating a new interface.... Sorry about my ignorance about the existing usage of XFS project quota. I hope it is widely used. But does it really matters for XFS that what kind of Ext4 interfaces is going to use? Existing appplications would run happily on XFS any way using the exisitng XFS interfaces. And if you are concerning about the compatibility between Ext4 and XFS, I am afraid those applications have to be changed any way when been ported to Ext4. Since those applications are using XFS specific feature, i.e. project quota, it is likely they are using other kind of XFS speicific features which probably will never be implemented on Ext4. I don't think there is any easy way to port them from XFS to Ext4 any way. And I really don't think therea are many such kind of applications. So, since we are not implementing interfaces for XFS2 or XFS3, I don't think compatibility problem is so critical. Regards, - Li Xi ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project @ 2014-09-26 2:45 ` Li Xi 0 siblings, 0 replies; 41+ messages in thread From: Li Xi @ 2014-09-26 2:45 UTC (permalink / raw) To: Dave Chinner Cc: Andreas Dilger, Jan Kara, linux-api, xfs, Christoph Hellwig, Dmitry Monakhov, viro, linux-fsdevel, Theodore Ts'o, Ext4 Developers List On Fri, Sep 26, 2014 at 8:10 AM, Dave Chinner <david@fromorbit.com> wrote: > On Thu, Sep 25, 2014 at 07:34:38PM +0800, lixi wrote: >> Hi Dave, >> >> I was mostly working on the semantics of inherit flag on these patches. And >> I didn’t realized that the interface differences would bother you so much. Sorry >> for that. > > It's not the differences that bother me - it's the fact that I was > repeatedly ignored until someone else raised the same issue.... Sorry about that. Since we are implementing a feature which exsits in XFS for years, your opions are surely very important. They are not ignored. But I think more discussion is necessary before an common agreement is reached. And I am willing to change the interfaces to whatever all people agree on. > >> I agree that we should choose a good interface. It would be good that it is >> general so that it works well for all file systems. I agree that adding an >> ext4 specific ioctl() is far from the best choice. I am willing to change it to >> any general interface. A general ioctl() sounds good to me. Extend setattr() >> and getattr() for project ID sounds even better, since project ID looks like >> UID/GID. > > Ah, no, project ID is not a uid/gid. It's a completely independent > construct. > > [ Which brings me to, once again, the issue of being ignored during > reviews: project IDs should not be mapped by user namespaces, nor be > accessible from anything other than the init namespace. > > In XFS we've turned off access to project IDs within namespace > containers because they are used for container space management > (i.e. by the init namespace) to manage the amount of filesystem > space a container or set of containers can use. We do not want > project IDs to be manipulated from within such containers, and > therefore have to prevent access to them from within user > namespaces. ] OK. That is another semantics which needs further discussion whether we need to implement for Ext4. Container space management is one of the project quota use case. I am not sure how widely used it is, but I like features with less limits. > >> And general xattr name is another choice. But it is might be a little >> bit confusing if we use xattr actually, since we are not saving project ID as >> extended attribute on Ext4. Any choice is fine with me, as long as the >> implementation won't introduce nasty codes or inconsistent design. > > We can easily create another ioctl name if we have to. It just needs > sto be defined to the same value as the XFS ioctl names currently > are. We've done this before when making ioctls that originated in > XFS generic (e.g. with freeze/thaw ioctls).... OK. I am fine with general ioctl to set/get project ID. > >> However, the problem is, I do not quite understand why we should keep >> the interface exactly the same with XFS. It would be good if we can. But >> as far as I can see, it seems hard. XFS uses a lot interfaces which are >> not so standard and used by other file systems. For example, struct >> fsxattr is not used by other file systems at all except XFS. > > Moving a existing structure definitions to a different header file > is too hard? I don't think difficulty is a big problem. I'm wondering whether it is necessary. > >> I am not sure why we should introduce this into Ext4 if there are >> a lot of other better ways. I would be happy to change to XFS >> interfaces, if it is general. However, I don’t think it is >> general enough. > > How is it not general enough? Examples, please, not handwaving: > which bit of the quota interface can't ext4 use because it's XFS > specific? Well, I am not so familar with XFS, so I might be all wrong about XFS. But honestly, a lot of quota interfaces of XFS seems not so standard. For example, all the Q_X* flags used in do_quotactl(). Maybe there are really good reasons why they are there. I don't know. And XFS does not use general codes under fs/quota. That is a big difference with Ext4. > > We already have a perfectly functional interface and a large body of > code that implements and tests it. You're saying "oh, it's too much > work for me to implement an existing interface" and ignoring the > fact that not implementing the existing interface forces a huge > amount of downstream work. e.g. > > - we need completely new test infrastructure to replicate > existing tests. > - we need new tests to ensure the different APIs and > utilities provide the same functionality, and that the > work identically. > - administrators are going to have to learn how ext4 is > different to what they already know and understand. > - administrators that has tools written to manage project > quotas is going to have to rewrite them to support ext4. > > It's an entirely selfish argument that ignores what already existing > out in userspace. i.e. you're saying that existing downstream users of > project quotas simple don't matter to you... > >> I know xfstest is using the existing project quota interfaces of XFS. And >> maybe there are some applications which are using them too. But >> keeping the interfaces exactly the same with XFS would cost so much >> effort that I’d like to get enough reasons before start working on it. Is it >> really necessary? I am not so sure. > > You have to have a stronger argument than that to justify creating a > new incompatible user interface. The XFS interfaces have been > available for more than 10 years and support all the functionality > ext4 requires. If it was any other userspace interface (e.g. > syscalls) or any filesystem other than ext4 there would be people > from all over telling you "use the existing interfaces!" and you'd > need very strong reasons for creating a new one. > > i.e. you need to demonstrate that the existing interfaces are > inadequate for the intended purpose of the new functionality. That's > clearly not the case here so why should we allow you to create an > incompatible userspace API rather than use the existing, fully > functional API? > >> It is so easy to change user space applications comparing to >> changing a weird interfaces. > > The existing generic quota tools (i.e quotactl, repquota, etc) > already implement the XFS quota API to be able to query XFS > filesystems. There's no "changing to wierd interfaces" necessary > for userspace; it's already all there. Hence any work you do to add > project quota awareness to those generic userspace tools will need to > add the support to the XFS queries anyway. > > IOWs, you're not making it any easier for yourself in userspace by > creating a new API for ext4 - it just doubles the amount of work you > have to in userspace to make existing tools project quota aware. I am afraid I have to make existing tools project quota aware. And actually I've done most of the work. I've updated e2fsprogs and quota-tools to support project quota. Unfortunately these updates are inevitable anyway. As Jan Kara said, we can't force system admins to change from quota-tool command to xfs quota tools. Thus, I add '-P $PROJECT' arguments to all the commands. And based on those tools, I made a script which is less than 1K lines for regression test. It is working pretty well. I don't see a good reason why it is necessary to change everything to XFS way. > >> For >> example, I think it won’t cost even more than a day to add xfstest >> support for new Ext4 project quota. > > A day of whose time? I am always willing to help if you agree. :) > > Ever thought about how much time it will take reviewers to look at > your tests and iterate over them to get it all right? If you're > introducing new userspace infrastructure that xfstests will need to > depend on and test for, then it's a lot more than just writing new > tests. > > Indeed, I'm likely to want new project quota tests to be generic > (i.e. works and passes on any filesystem that supports project > quotas) with the introduction of ext4 project quota support. It's > the same functionality and so it should work the same just like user > and group quotas do across all filesystems. > >> And since project quota is far from >> a widely used feature, > > I don't think you realise quite how widespread it's use is on XFS. > >> I don’t think there is much compatibility problems >> for existing applications. And If the new project interface are general >> enough, there won’t be any compatibility problems for new applications >> at all. > > Again, you are ignoring the compatibility problems with existing > applications that are project quota aware. For them you are > *creating new compatibility problems* by implementing a new > interface. i.e. Existing applications will not work on ext4, and > new applications written to work on ext4 won't work on XFS. > > That's the crux of the issue - we have existing applications using > the existing interface and so introducing a new interface introduces > compatibility problems. You can't just wave this problem away > because you don't think the existing interface matters. > > "It's easier for me to create a new interface" is not a valid reason > for creating a new interface.... Sorry about my ignorance about the existing usage of XFS project quota. I hope it is widely used. But does it really matters for XFS that what kind of Ext4 interfaces is going to use? Existing appplications would run happily on XFS any way using the exisitng XFS interfaces. And if you are concerning about the compatibility between Ext4 and XFS, I am afraid those applications have to be changed any way when been ported to Ext4. Since those applications are using XFS specific feature, i.e. project quota, it is likely they are using other kind of XFS speicific features which probably will never be implemented on Ext4. I don't think there is any easy way to port them from XFS to Ext4 any way. And I really don't think therea are many such kind of applications. So, since we are not implementing interfaces for XFS2 or XFS3, I don't think compatibility problem is so critical. Regards, - Li Xi _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project 2014-09-25 7:59 ` Dave Chinner 2014-09-25 11:34 ` lixi @ 2014-09-25 13:41 ` Theodore Ts'o [not found] ` <20140925134136.GE4592-AKGzg7BKzIDYtjvyW6yDsg@public.gmane.org> 2014-09-25 13:52 ` Jan Kara 2 siblings, 1 reply; 41+ messages in thread From: Theodore Ts'o @ 2014-09-25 13:41 UTC (permalink / raw) To: Dave Chinner Cc: adilger, Jan Kara, linux-api, xfs, Christoph Hellwig, dmonakhov, viro, Li Xi, linux-fsdevel, linux-ext4 On Thu, Sep 25, 2014 at 05:59:12PM +1000, Dave Chinner wrote: > > Also I'm afraid we may quickly run out of > > 32 available flags in xflags so we'd need to extend that. But all this > > seems to be doable. > > The struct fsxattr was designed to be extensible - it has unused > padding and enough space in the flags field to allow us to > conditionally use that padding.... I agree that it would be useful for ext4 to support as much of the XFS_IOC_GETXATTR/XFS_IOC_SETATTR as would make sense for ext4, and to use that to set/get the project ID. (And that we should probably do that as a separate set of patches that we could potentially go into ext4 ahead of the project quota while it is undergoing testing and review.) A few questions of Dave and other XFS folks: 1) If we only implement a partial set of the flags or other functionality, are there going to be tools that get confused? i.e., are there any userspace programs that will test for whether the ioctl is supported, and then assume that some minimal set of functionality must be implemented? 2) Unless I'm missing something, there is nothing that enforces that fsx_pad must be zero. I assume that means that the only way you can expand use of fields into that space is via a flag bit being consumed? Cheers, - Ted _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 41+ messages in thread
[parent not found: <20140925134136.GE4592-AKGzg7BKzIDYtjvyW6yDsg@public.gmane.org>]
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project 2014-09-25 13:41 ` Theodore Ts'o @ 2014-09-25 22:22 ` Dave Chinner 0 siblings, 0 replies; 41+ messages in thread From: Dave Chinner @ 2014-09-25 22:22 UTC (permalink / raw) To: Theodore Ts'o Cc: Jan Kara, Christoph Hellwig, adilger-m1MBpc4rdrD3fQ9qLvQP4Q, linux-api-u79uwXL29TY76Z2rM5mHXA, xfs-VZNHf3L845pBDgjK7y7TUQ, dmonakhov-GEFAQzZX7r8dnm+yROfE0A, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, Li Xi, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-ext4-u79uwXL29TY76Z2rM5mHXA On Thu, Sep 25, 2014 at 09:41:37AM -0400, Theodore Ts'o wrote: > On Thu, Sep 25, 2014 at 05:59:12PM +1000, Dave Chinner wrote: > > > Also I'm afraid we may quickly run out of > > > 32 available flags in xflags so we'd need to extend that. But all this > > > seems to be doable. > > > > The struct fsxattr was designed to be extensible - it has unused > > padding and enough space in the flags field to allow us to > > conditionally use that padding.... > > I agree that it would be useful for ext4 to support as much of the > XFS_IOC_GETXATTR/XFS_IOC_SETATTR as would make sense for ext4, and to > use that to set/get the project ID. (And that we should probably do > that as a separate set of patches that we could potentially go into > ext4 ahead of the project quota while it is undergoing testing and > review.) > > A few questions of Dave and other XFS folks: > > 1) If we only implement a partial set of the flags or other > functionality, are there going to be tools that get confused? i.e., > are there any userspace programs that will test for whether the ioctl > is supported, and then assume that some minimal set of functionality > must be implemented? No, I don't think they will get confused. The use of the flags is get/modify/set just like other flag setting functions. The extsize and projid fields are condition on the relevant flag being set on return from a get (i.e. projid is only valid if XFS_XFLAG_PROJID[_INHERIT] is set), and those fields are only considered valid on set if those flags are set by the application (or remain set as a result of the getxattr). Hence the applications that use the getxattr/setxattr interface correctly shouldn't care what set of flags and values the filesystem supports other than the specific flags the application needs the filesystem to understand. > 2) Unless I'm missing something, there is nothing that enforces that > fsx_pad must be zero. I assume that means that the only way you can > expand use of fields into that space is via a flag bit being consumed? Yup, that's exactly what I meant by "conditionally use the padding". Even if the padding was guaranteed to be zero, I'd strongly recommend a flag bit to indicate the application understands that the padding region has actual meaning to guard against buggy applications. Cheers, Dave. -- Dave Chinner david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project @ 2014-09-25 22:22 ` Dave Chinner 0 siblings, 0 replies; 41+ messages in thread From: Dave Chinner @ 2014-09-25 22:22 UTC (permalink / raw) To: Theodore Ts'o Cc: adilger, Jan Kara, linux-api, xfs, Christoph Hellwig, dmonakhov, viro, Li Xi, linux-fsdevel, linux-ext4 On Thu, Sep 25, 2014 at 09:41:37AM -0400, Theodore Ts'o wrote: > On Thu, Sep 25, 2014 at 05:59:12PM +1000, Dave Chinner wrote: > > > Also I'm afraid we may quickly run out of > > > 32 available flags in xflags so we'd need to extend that. But all this > > > seems to be doable. > > > > The struct fsxattr was designed to be extensible - it has unused > > padding and enough space in the flags field to allow us to > > conditionally use that padding.... > > I agree that it would be useful for ext4 to support as much of the > XFS_IOC_GETXATTR/XFS_IOC_SETATTR as would make sense for ext4, and to > use that to set/get the project ID. (And that we should probably do > that as a separate set of patches that we could potentially go into > ext4 ahead of the project quota while it is undergoing testing and > review.) > > A few questions of Dave and other XFS folks: > > 1) If we only implement a partial set of the flags or other > functionality, are there going to be tools that get confused? i.e., > are there any userspace programs that will test for whether the ioctl > is supported, and then assume that some minimal set of functionality > must be implemented? No, I don't think they will get confused. The use of the flags is get/modify/set just like other flag setting functions. The extsize and projid fields are condition on the relevant flag being set on return from a get (i.e. projid is only valid if XFS_XFLAG_PROJID[_INHERIT] is set), and those fields are only considered valid on set if those flags are set by the application (or remain set as a result of the getxattr). Hence the applications that use the getxattr/setxattr interface correctly shouldn't care what set of flags and values the filesystem supports other than the specific flags the application needs the filesystem to understand. > 2) Unless I'm missing something, there is nothing that enforces that > fsx_pad must be zero. I assume that means that the only way you can > expand use of fields into that space is via a flag bit being consumed? Yup, that's exactly what I meant by "conditionally use the padding". Even if the padding was guaranteed to be zero, I'd strongly recommend a flag bit to indicate the application understands that the padding region has actual meaning to guard against buggy applications. Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project 2014-09-25 7:59 ` Dave Chinner @ 2014-09-25 13:52 ` Jan Kara 2014-09-25 13:41 ` Theodore Ts'o 2014-09-25 13:52 ` Jan Kara 2 siblings, 0 replies; 41+ messages in thread From: Jan Kara @ 2014-09-25 13:52 UTC (permalink / raw) To: Dave Chinner Cc: Jan Kara, Christoph Hellwig, adilger-m1MBpc4rdrD3fQ9qLvQP4Q, linux-api-u79uwXL29TY76Z2rM5mHXA, xfs-VZNHf3L845pBDgjK7y7TUQ, dmonakhov-GEFAQzZX7r8dnm+yROfE0A, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, Li Xi, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, tytso-3s7WtUTddSA, linux-ext4-u79uwXL29TY76Z2rM5mHXA On Thu 25-09-14 17:59:12, Dave Chinner wrote: > On Wed, Sep 24, 2014 at 07:01:05PM +0200, Jan Kara wrote: > > On Wed 24-09-14 09:26:34, Christoph Hellwig wrote: > > > On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote: > > > > On Wed 24-09-14 22:04:30, Li Xi wrote: > > > > > This patch adds ioctl interface for setting/getting project of ext4. > > > > The patch looks good to me. I was just wondering whether it won't be > > > > useful to add an ioctl() which isn't ext4 specific. We could just extend > > > > ->setattr() to allow setting of project ID (most filesystems would just > > > > return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call > > > > ->setattr from the generic ioctl. That way userspace won't have to care > > > > about filesystem type when setting project ID... What do others think? > > > > > > Absolutely. In general I also wonder why this patch doesn't implement > > > the full XFS API. Maybe there is a reason it was considered and > > > rejected, but it would be helpful to document why. > > Do you mean full get/setfsxattr API? > > That's a good start. > > The bigger issue in my mind is that we already have a fully featured > quota API that supports project quotas and userspace tools available > that manipulate it. xfstests already uses those tools and API > for testing project quotas. Well, the VFS quota API is trivially extended by adding additional quota type so I don't really see about which reinventing of quota API are you speaking here... > This whole patchset reinvents all the quota APIs, and will require > adding support in userspace, and hence require re-inventing all the > test infrastructure we already have because it won't be compatible > with the existing project quota test code. Well, quota-tools will have to extended to know about the new quota type. Yes. But that's easy to do. I think teaching xfs quota tools to work with ext4 will be a bigger project plus I don't think I want to force sysadmins which are used to work with quota-tools to switch to other utilities just because of project quotas. Regarding xfstests - I've checked and most of the project quota tests in xfs directory aren't directly usable for ext4 anyway because of other functionality ext4 doesn't support. So we'll need to distill the least common denominator from them anyway... > > That basically contains project ID, > > flags (those that are currently get/set with FS_IOC_GETFLAGS/SETFLAGS), and > > extent size hint right? > > It's a different set of flag definitions. We translate the interface > XFS_XFLAG_* values to/from the inode on-disk XFS_DIFLAG_* inode values, just > like we translate the VFS FS_*_FL flags that get passed through the > FS_IOC_GETFLAGS/SETFLAGS ioctl. > > > That seems workable and it would also make setting > > of PROJINHERIT flag fs agnostic. Only we would have to create some generic > > flags namespace and merge into that ext4 flags and have a translation > > function for the old ext4 flags. > > The XFS_XFLAGS_* are already filesystem agnostic - they are flags > that are only used for interfacing with userspace and hence only > exist at the ioctl copy in/out layer..... > > > Also I'm afraid we may quickly run out of > > 32 available flags in xflags so we'd need to extend that. But all this > > seems to be doable. > > The struct fsxattr was designed to be extensible - it has unused > padding and enough space in the flags field to allow us to > conditionally use that padding.... Yeah, this should all be easy. Honza -- Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> SUSE Labs, CR ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project @ 2014-09-25 13:52 ` Jan Kara 0 siblings, 0 replies; 41+ messages in thread From: Jan Kara @ 2014-09-25 13:52 UTC (permalink / raw) To: Dave Chinner Cc: adilger, Jan Kara, linux-api, xfs, Christoph Hellwig, dmonakhov, viro, Li Xi, linux-fsdevel, tytso, linux-ext4 On Thu 25-09-14 17:59:12, Dave Chinner wrote: > On Wed, Sep 24, 2014 at 07:01:05PM +0200, Jan Kara wrote: > > On Wed 24-09-14 09:26:34, Christoph Hellwig wrote: > > > On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote: > > > > On Wed 24-09-14 22:04:30, Li Xi wrote: > > > > > This patch adds ioctl interface for setting/getting project of ext4. > > > > The patch looks good to me. I was just wondering whether it won't be > > > > useful to add an ioctl() which isn't ext4 specific. We could just extend > > > > ->setattr() to allow setting of project ID (most filesystems would just > > > > return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call > > > > ->setattr from the generic ioctl. That way userspace won't have to care > > > > about filesystem type when setting project ID... What do others think? > > > > > > Absolutely. In general I also wonder why this patch doesn't implement > > > the full XFS API. Maybe there is a reason it was considered and > > > rejected, but it would be helpful to document why. > > Do you mean full get/setfsxattr API? > > That's a good start. > > The bigger issue in my mind is that we already have a fully featured > quota API that supports project quotas and userspace tools available > that manipulate it. xfstests already uses those tools and API > for testing project quotas. Well, the VFS quota API is trivially extended by adding additional quota type so I don't really see about which reinventing of quota API are you speaking here... > This whole patchset reinvents all the quota APIs, and will require > adding support in userspace, and hence require re-inventing all the > test infrastructure we already have because it won't be compatible > with the existing project quota test code. Well, quota-tools will have to extended to know about the new quota type. Yes. But that's easy to do. I think teaching xfs quota tools to work with ext4 will be a bigger project plus I don't think I want to force sysadmins which are used to work with quota-tools to switch to other utilities just because of project quotas. Regarding xfstests - I've checked and most of the project quota tests in xfs directory aren't directly usable for ext4 anyway because of other functionality ext4 doesn't support. So we'll need to distill the least common denominator from them anyway... > > That basically contains project ID, > > flags (those that are currently get/set with FS_IOC_GETFLAGS/SETFLAGS), and > > extent size hint right? > > It's a different set of flag definitions. We translate the interface > XFS_XFLAG_* values to/from the inode on-disk XFS_DIFLAG_* inode values, just > like we translate the VFS FS_*_FL flags that get passed through the > FS_IOC_GETFLAGS/SETFLAGS ioctl. > > > That seems workable and it would also make setting > > of PROJINHERIT flag fs agnostic. Only we would have to create some generic > > flags namespace and merge into that ext4 flags and have a translation > > function for the old ext4 flags. > > The XFS_XFLAGS_* are already filesystem agnostic - they are flags > that are only used for interfacing with userspace and hence only > exist at the ioctl copy in/out layer..... > > > Also I'm afraid we may quickly run out of > > 32 available flags in xflags so we'd need to extend that. But all this > > seems to be doable. > > The struct fsxattr was designed to be extensible - it has unused > padding and enough space in the flags field to allow us to > conditionally use that padding.... Yeah, this should all be easy. Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project 2014-09-25 13:52 ` Jan Kara (?) @ 2014-09-25 22:42 ` Dave Chinner 2014-09-26 12:01 ` Theodore Ts'o 2014-09-29 15:55 ` Jan Kara -1 siblings, 2 replies; 41+ messages in thread From: Dave Chinner @ 2014-09-25 22:42 UTC (permalink / raw) To: Jan Kara Cc: adilger, tytso, linux-api, xfs, Christoph Hellwig, dmonakhov, viro, Li Xi, linux-fsdevel, linux-ext4 On Thu, Sep 25, 2014 at 03:52:13PM +0200, Jan Kara wrote: > On Thu 25-09-14 17:59:12, Dave Chinner wrote: > > On Wed, Sep 24, 2014 at 07:01:05PM +0200, Jan Kara wrote: > > > On Wed 24-09-14 09:26:34, Christoph Hellwig wrote: > > > > On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote: > > > > > On Wed 24-09-14 22:04:30, Li Xi wrote: > > > > > > This patch adds ioctl interface for setting/getting project of ext4. > > > > > The patch looks good to me. I was just wondering whether it won't be > > > > > useful to add an ioctl() which isn't ext4 specific. We could just extend > > > > > ->setattr() to allow setting of project ID (most filesystems would just > > > > > return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call > > > > > ->setattr from the generic ioctl. That way userspace won't have to care > > > > > about filesystem type when setting project ID... What do others think? > > > > > > > > Absolutely. In general I also wonder why this patch doesn't implement > > > > the full XFS API. Maybe there is a reason it was considered and > > > > rejected, but it would be helpful to document why. > > > Do you mean full get/setfsxattr API? > > > > That's a good start. > > > > The bigger issue in my mind is that we already have a fully featured > > quota API that supports project quotas and userspace tools available > > that manipulate it. xfstests already uses those tools and API > > for testing project quotas. > Well, the VFS quota API is trivially extended by adding additional quota > type so I don't really see about which reinventing of quota API are you > speaking here... It doesn't seem quite as trivial as you make out given all thei issues so far around increasing MAXQUOTA, the increase in size of the inode, etc. > > This whole patchset reinvents all the quota APIs, and will require > > adding support in userspace, and hence require re-inventing all the > > test infrastructure we already have because it won't be compatible > > with the existing project quota test code. > Well, quota-tools will have to extended to know about the new quota type. > Yes. But that's easy to do. I think teaching xfs quota tools to work with > ext4 will be a bigger project plus I don't think I want to force sysadmins > which are used to work with quota-tools to switch to other utilities just > because of project quotas. > > Regarding xfstests - I've checked and most of the project quota tests in > xfs directory aren't directly usable for ext4 anyway because of other > functionality ext4 doesn't support. So we'll need to distill the least > common denominator from them anyway... I just did a quick scan - of the ~13 tests in tests/xfs that exercise project quotas, only 2 of them test things that are xfs specific (e.g. use xfs_db to peer at things, or use xfs_admin, etc). The rest all rely on xfs_quota to manage and configure project quotas but otherwise don't do anything XFS specific. We want project quotas to have the same management interface for administrators regardless of the filesystem they are using. The only way we can do that is to ensure that the same tools work on either filesystem, and right now it seems to me that the ext4 NIH syndrome is winning out over what is best for our users... Look, I have no problems with extending the existing quota interfaces to support project quotas, but that should be a *secondary* improvement as userspace tools are updated. The primary goal needs to be "works identically to XFS" and so it needs to implement the interfaces that are currently used for management so that we can actually test that it does work identically. Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project 2014-09-25 22:42 ` Dave Chinner @ 2014-09-26 12:01 ` Theodore Ts'o 2014-09-29 15:55 ` Jan Kara 1 sibling, 0 replies; 41+ messages in thread From: Theodore Ts'o @ 2014-09-26 12:01 UTC (permalink / raw) To: Dave Chinner Cc: Jan Kara, Christoph Hellwig, adilger, linux-api, xfs, dmonakhov, viro, Li Xi, linux-fsdevel, linux-ext4 On Fri, Sep 26, 2014 at 08:42:25AM +1000, Dave Chinner wrote: > Look, I have no problems with extending the existing quota > interfaces to support project quotas, but that should be a > *secondary* improvement as userspace tools are updated. The > primary goal needs to be "works identically to XFS" and so it needs > to implement the interfaces that are currently used for management > so that we can actually test that it does work identically. I think we're getting a bit too hung up on which is the "primary" and which is the "secondary" interfaces. The reality is that we should make both interfaces work. An example of this is how we handle the xfs-specific ioctls that are also exposed via the fallocate(2) system call. It's not particularly important to me which is the "primary" interface just because it's been around for 10 years. Which should implement *both* so that users are used to using xfs_io(8) or fallocate(1) can do what they want. Similarly, if some users are more used to the quotatools interface (which has been around for quite a long time, BTW, if we're trying to count primacy by years of availability to Linux users), and some users that are used to using xfs_quota, both should work. Cheers, - Ted ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project @ 2014-09-26 12:01 ` Theodore Ts'o 0 siblings, 0 replies; 41+ messages in thread From: Theodore Ts'o @ 2014-09-26 12:01 UTC (permalink / raw) To: Dave Chinner Cc: adilger, Jan Kara, linux-api, xfs, Christoph Hellwig, dmonakhov, viro, Li Xi, linux-fsdevel, linux-ext4 On Fri, Sep 26, 2014 at 08:42:25AM +1000, Dave Chinner wrote: > Look, I have no problems with extending the existing quota > interfaces to support project quotas, but that should be a > *secondary* improvement as userspace tools are updated. The > primary goal needs to be "works identically to XFS" and so it needs > to implement the interfaces that are currently used for management > so that we can actually test that it does work identically. I think we're getting a bit too hung up on which is the "primary" and which is the "secondary" interfaces. The reality is that we should make both interfaces work. An example of this is how we handle the xfs-specific ioctls that are also exposed via the fallocate(2) system call. It's not particularly important to me which is the "primary" interface just because it's been around for 10 years. Which should implement *both* so that users are used to using xfs_io(8) or fallocate(1) can do what they want. Similarly, if some users are more used to the quotatools interface (which has been around for quite a long time, BTW, if we're trying to count primacy by years of availability to Linux users), and some users that are used to using xfs_quota, both should work. Cheers, - Ted _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project 2014-09-25 22:42 ` Dave Chinner @ 2014-09-29 15:55 ` Jan Kara 2014-09-29 15:55 ` Jan Kara 1 sibling, 0 replies; 41+ messages in thread From: Jan Kara @ 2014-09-29 15:55 UTC (permalink / raw) To: Dave Chinner Cc: Jan Kara, Christoph Hellwig, adilger-m1MBpc4rdrD3fQ9qLvQP4Q, linux-api-u79uwXL29TY76Z2rM5mHXA, xfs-VZNHf3L845pBDgjK7y7TUQ, dmonakhov-GEFAQzZX7r8dnm+yROfE0A, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, Li Xi, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, tytso-3s7WtUTddSA, linux-ext4-u79uwXL29TY76Z2rM5mHXA On Fri 26-09-14 08:42:25, Dave Chinner wrote: > On Thu, Sep 25, 2014 at 03:52:13PM +0200, Jan Kara wrote: > > On Thu 25-09-14 17:59:12, Dave Chinner wrote: > > > On Wed, Sep 24, 2014 at 07:01:05PM +0200, Jan Kara wrote: > > > > On Wed 24-09-14 09:26:34, Christoph Hellwig wrote: > > > > > On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote: > > > > > > On Wed 24-09-14 22:04:30, Li Xi wrote: > > > > > > > This patch adds ioctl interface for setting/getting project of ext4. > > > > > > The patch looks good to me. I was just wondering whether it won't be > > > > > > useful to add an ioctl() which isn't ext4 specific. We could just extend > > > > > > ->setattr() to allow setting of project ID (most filesystems would just > > > > > > return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call > > > > > > ->setattr from the generic ioctl. That way userspace won't have to care > > > > > > about filesystem type when setting project ID... What do others think? > > > > > > > > > > Absolutely. In general I also wonder why this patch doesn't implement > > > > > the full XFS API. Maybe there is a reason it was considered and > > > > > rejected, but it would be helpful to document why. > > > > Do you mean full get/setfsxattr API? > > > > > > That's a good start. > > > > > > The bigger issue in my mind is that we already have a fully featured > > > quota API that supports project quotas and userspace tools available > > > that manipulate it. xfstests already uses those tools and API > > > for testing project quotas. > > Well, the VFS quota API is trivially extended by adding additional quota > > type so I don't really see about which reinventing of quota API are you > > speaking here... > > It doesn't seem quite as trivial as you make out given all thei > issues so far around increasing MAXQUOTA, the increase in size of > the inode, etc. Well, troubles with increasing MAXQUOTAS is more about expectations that VFS quota subsystem can support only 2 quota types. So we have to do those changes regardless of interface we choose. There is one change necessary in the interface (not done yet) and that is that filesystems using VFS quotas and not supporting project quotas will need to refuse quotactls for project quotas. This won't be necessary if we simply refuse to manipulate project quotas using the standard VFS interface. But I wouldn't call it difficult either. > > > This whole patchset reinvents all the quota APIs, and will require > > > adding support in userspace, and hence require re-inventing all the > > > test infrastructure we already have because it won't be compatible > > > with the existing project quota test code. > > Well, quota-tools will have to extended to know about the new quota type. > > Yes. But that's easy to do. I think teaching xfs quota tools to work with > > ext4 will be a bigger project plus I don't think I want to force sysadmins > > which are used to work with quota-tools to switch to other utilities just > > because of project quotas. > > > > Regarding xfstests - I've checked and most of the project quota tests in > > xfs directory aren't directly usable for ext4 anyway because of other > > functionality ext4 doesn't support. So we'll need to distill the least > > common denominator from them anyway... > > I just did a quick scan - of the ~13 tests in tests/xfs that > exercise project quotas, only 2 of them test things that are xfs > specific (e.g. use xfs_db to peer at things, or use xfs_admin, etc). > The rest all rely on xfs_quota to manage and configure project > quotas but otherwise don't do anything XFS specific. Yeah, I messed up my original check (I originally found like 5 project quota related tests and they were those problematic). I checked again and most of them should be relatively easy to adapt (we'll need some changes for mount options handling but that's inevitable). > We want project quotas to have the same management interface for > administrators regardless of the filesystem they are using. The only > way we can do that is to ensure that the same tools work on either > filesystem, and right now it seems to me that the ext4 NIH syndrome > is winning out over what is best for our users... > > Look, I have no problems with extending the existing quota > interfaces to support project quotas, but that should be a > *secondary* improvement as userspace tools are updated. The > primary goal needs to be "works identically to XFS" and so it needs > to implement the interfaces that are currently used for management > so that we can actually test that it does work identically. So I had another look at the quotactl interface we are speaking about. XFS has the following commands there: Q_XQUOTAON Q_XQUOTAOFF These could be relatively easily hooked up to call appropriate VFS functions. Q_XQUOTARM This doesn't have equivalent in VFS and currently I'm not convinced we want to do the work in filesystems to support this... Q_XGETQSTAT Q_XGETQSTATV This corresponds to Q_GETINFO of VFS quotas although it provides more information. We don't easily have things like number of incore dquots or number of file extents available. There's also no limit on the number of warnings. But other than that diverting these to VFS interfaces through a translation function should be easy. Any idea on what numbers should we present from VFS? BTW how do you set the information? We have Q_SETINFO for that in VFS quotas. Q_XSETQLIM Q_XGETQUOTA These are already handled so they work regardless of the underlying fs type. Q_XQUOTASYNC This is the same as Q_SYNC. For VFS quotas we need to do some work in some cases. So all in all it seems relatively easy to make the VFS and XFS quota interfaces more compatible than they are now and it's a direction I like. I can have a look into that once I finish patches to move i_dquot[] array out of generic inode (which is desirable regardless of project quotas). Honza -- Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> SUSE Labs, CR ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project @ 2014-09-29 15:55 ` Jan Kara 0 siblings, 0 replies; 41+ messages in thread From: Jan Kara @ 2014-09-29 15:55 UTC (permalink / raw) To: Dave Chinner Cc: adilger, Jan Kara, linux-api, xfs, Christoph Hellwig, dmonakhov, viro, Li Xi, linux-fsdevel, tytso, linux-ext4 On Fri 26-09-14 08:42:25, Dave Chinner wrote: > On Thu, Sep 25, 2014 at 03:52:13PM +0200, Jan Kara wrote: > > On Thu 25-09-14 17:59:12, Dave Chinner wrote: > > > On Wed, Sep 24, 2014 at 07:01:05PM +0200, Jan Kara wrote: > > > > On Wed 24-09-14 09:26:34, Christoph Hellwig wrote: > > > > > On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote: > > > > > > On Wed 24-09-14 22:04:30, Li Xi wrote: > > > > > > > This patch adds ioctl interface for setting/getting project of ext4. > > > > > > The patch looks good to me. I was just wondering whether it won't be > > > > > > useful to add an ioctl() which isn't ext4 specific. We could just extend > > > > > > ->setattr() to allow setting of project ID (most filesystems would just > > > > > > return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call > > > > > > ->setattr from the generic ioctl. That way userspace won't have to care > > > > > > about filesystem type when setting project ID... What do others think? > > > > > > > > > > Absolutely. In general I also wonder why this patch doesn't implement > > > > > the full XFS API. Maybe there is a reason it was considered and > > > > > rejected, but it would be helpful to document why. > > > > Do you mean full get/setfsxattr API? > > > > > > That's a good start. > > > > > > The bigger issue in my mind is that we already have a fully featured > > > quota API that supports project quotas and userspace tools available > > > that manipulate it. xfstests already uses those tools and API > > > for testing project quotas. > > Well, the VFS quota API is trivially extended by adding additional quota > > type so I don't really see about which reinventing of quota API are you > > speaking here... > > It doesn't seem quite as trivial as you make out given all thei > issues so far around increasing MAXQUOTA, the increase in size of > the inode, etc. Well, troubles with increasing MAXQUOTAS is more about expectations that VFS quota subsystem can support only 2 quota types. So we have to do those changes regardless of interface we choose. There is one change necessary in the interface (not done yet) and that is that filesystems using VFS quotas and not supporting project quotas will need to refuse quotactls for project quotas. This won't be necessary if we simply refuse to manipulate project quotas using the standard VFS interface. But I wouldn't call it difficult either. > > > This whole patchset reinvents all the quota APIs, and will require > > > adding support in userspace, and hence require re-inventing all the > > > test infrastructure we already have because it won't be compatible > > > with the existing project quota test code. > > Well, quota-tools will have to extended to know about the new quota type. > > Yes. But that's easy to do. I think teaching xfs quota tools to work with > > ext4 will be a bigger project plus I don't think I want to force sysadmins > > which are used to work with quota-tools to switch to other utilities just > > because of project quotas. > > > > Regarding xfstests - I've checked and most of the project quota tests in > > xfs directory aren't directly usable for ext4 anyway because of other > > functionality ext4 doesn't support. So we'll need to distill the least > > common denominator from them anyway... > > I just did a quick scan - of the ~13 tests in tests/xfs that > exercise project quotas, only 2 of them test things that are xfs > specific (e.g. use xfs_db to peer at things, or use xfs_admin, etc). > The rest all rely on xfs_quota to manage and configure project > quotas but otherwise don't do anything XFS specific. Yeah, I messed up my original check (I originally found like 5 project quota related tests and they were those problematic). I checked again and most of them should be relatively easy to adapt (we'll need some changes for mount options handling but that's inevitable). > We want project quotas to have the same management interface for > administrators regardless of the filesystem they are using. The only > way we can do that is to ensure that the same tools work on either > filesystem, and right now it seems to me that the ext4 NIH syndrome > is winning out over what is best for our users... > > Look, I have no problems with extending the existing quota > interfaces to support project quotas, but that should be a > *secondary* improvement as userspace tools are updated. The > primary goal needs to be "works identically to XFS" and so it needs > to implement the interfaces that are currently used for management > so that we can actually test that it does work identically. So I had another look at the quotactl interface we are speaking about. XFS has the following commands there: Q_XQUOTAON Q_XQUOTAOFF These could be relatively easily hooked up to call appropriate VFS functions. Q_XQUOTARM This doesn't have equivalent in VFS and currently I'm not convinced we want to do the work in filesystems to support this... Q_XGETQSTAT Q_XGETQSTATV This corresponds to Q_GETINFO of VFS quotas although it provides more information. We don't easily have things like number of incore dquots or number of file extents available. There's also no limit on the number of warnings. But other than that diverting these to VFS interfaces through a translation function should be easy. Any idea on what numbers should we present from VFS? BTW how do you set the information? We have Q_SETINFO for that in VFS quotas. Q_XSETQLIM Q_XGETQUOTA These are already handled so they work regardless of the underlying fs type. Q_XQUOTASYNC This is the same as Q_SYNC. For VFS quotas we need to do some work in some cases. So all in all it seems relatively easy to make the VFS and XFS quota interfaces more compatible than they are now and it's a direction I like. I can have a look into that once I finish patches to move i_dquot[] array out of generic inode (which is desirable regardless of project quotas). Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project 2014-09-24 16:25 ` Jan Kara (?) (?) @ 2014-09-25 7:26 ` Dave Chinner -1 siblings, 0 replies; 41+ messages in thread From: Dave Chinner @ 2014-09-25 7:26 UTC (permalink / raw) To: Jan Kara Cc: adilger, tytso, linux-api, xfs, hch, dmonakhov, viro, Li Xi, linux-fsdevel, linux-ext4 On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote: > On Wed 24-09-14 22:04:30, Li Xi wrote: > > This patch adds ioctl interface for setting/getting project of ext4. > The patch looks good to me. I was just wondering whether it won't be > useful to add an ioctl() which isn't ext4 specific. We could just extend > ->setattr() to allow setting of project ID (most filesystems would just > return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call > ->setattr from the generic ioctl. That way userspace won't have to care > about filesystem type when setting project ID... What do others think? I've repeatedly said that these ext4 project ID patches should implement the same interfaces as XFS rather than creating a new set of incompatible, ext4 specific interfaces to do implement the same functionality. There is no good reason for forcing userspace to re-invent tools that already exist just to manage identical functionality in different filesystems. Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 41+ messages in thread
[parent not found: <1411567470-31799-5-git-send-email-lixi-LfVdkaOWEx8@public.gmane.org>]
* Re: [PATCH 4/4] Adds ioctl interface support for ext4 project [not found] ` <1411567470-31799-5-git-send-email-lixi-LfVdkaOWEx8@public.gmane.org> @ 2014-09-24 16:43 ` Jan Kara 0 siblings, 0 replies; 41+ messages in thread From: Jan Kara @ 2014-09-24 16:43 UTC (permalink / raw) To: Li Xi Cc: linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-ext4-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA, tytso-3s7WtUTddSA, adilger-m1MBpc4rdrD3fQ9qLvQP4Q, jack-AlSwsSmVLrQ, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, hch-wEGCiKHe2LqWVfeAwA7xHQ, dmonakhov-GEFAQzZX7r8dnm+yROfE0A On Wed 24-09-14 22:04:30, Li Xi wrote: > This patch adds ioctl interface for setting/getting project of ext4. One more thing... > Signed-off-by: Li Xi <lixi-LfVdkaOWEx8@public.gmane.org> > --- > Documentation/filesystems/ext4.txt | 4 ++ > fs/ext4/ext4.h | 2 + > fs/ext4/ioctl.c | 85 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 91 insertions(+), 0 deletions(-) > > + case EXT4_IOC_SETPROJECT: > + { > + __u32 projid; > + int err; > + handle_t *handle; > + kprojid_t kprojid; > + struct ext4_iloc iloc; > + struct ext4_inode *raw_inode; > + > + struct dquot *transfer_to[EXT4_MAXQUOTAS] = { }; > + > + /* Make sure caller can change project. */ > + if (!capable(CAP_SYS_ADMIN)) > + return -EACCES; I realized you forgot to check EXT4_FEATURE_RO_COMPAT_PROJECT here... Honza > + > + if (get_user(projid, (__u32 __user *) arg)) > + return -EFAULT; > + > + kprojid = make_kprojid(&init_user_ns, (projid_t)projid); > + > + if (projid_eq(kprojid, EXT4_I(inode)->i_projid)) > + return 0; > + > + err = mnt_want_write_file(filp); > + if (err) > + return err; > + > + err = -EPERM; > + mutex_lock(&inode->i_mutex); > + /* Is it quota file? Do not allow user to mess with it */ > + if (IS_NOQUOTA(inode)) > + goto project_out; > + > + dquot_initialize(inode); > + > + handle = ext4_journal_start(inode, EXT4_HT_QUOTA, > + EXT4_QUOTA_INIT_BLOCKS(sb) + > + EXT4_QUOTA_DEL_BLOCKS(sb) + 3); > + if (IS_ERR(handle)) { > + err = PTR_ERR(handle); > + goto project_out; > + } > + > + err = ext4_reserve_inode_write(handle, inode, &iloc); > + if (err) > + goto project_stop; > + > + raw_inode = ext4_raw_inode(&iloc); > + if ((EXT4_INODE_SIZE(inode->i_sb) <= > + EXT4_GOOD_OLD_INODE_SIZE) || > + (!EXT4_FITS_IN_INODE(raw_inode, ei, i_projid))) { > + err = -EFBIG; > + goto project_stop; > + } > + > + transfer_to[PRJQUOTA] = dqget(sb, make_kqid_projid(kprojid)); > + if (!transfer_to[PRJQUOTA]) > + goto project_set; > + > + err = __dquot_transfer(inode, transfer_to); > + dqput(transfer_to[PRJQUOTA]); > + if (err) > + goto project_stop; > + > +project_set: > + EXT4_I(inode)->i_projid = kprojid; > + inode->i_ctime = ext4_current_time(inode); > + err = ext4_mark_iloc_dirty(handle, inode, &iloc); > +project_stop: > + ext4_journal_stop(handle); > +project_out: > + mutex_unlock(&inode->i_mutex); > + mnt_drop_write_file(filp); > + return err; > + } > default: > return -ENOTTY; > } > -- > 1.7.1 > -- Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> SUSE Labs, CR ^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2014-09-30 20:07 UTC | newest] Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-09-24 14:04 [PATCH 0/4] quota: add project quota support Li Xi 2014-09-24 14:04 ` [PATCH 1/4] Adds general codes to enforces project quota limits Li Xi 2014-09-24 16:08 ` Jan Kara [not found] ` <1411567470-31799-2-git-send-email-lixi-LfVdkaOWEx8@public.gmane.org> 2014-09-24 16:14 ` Christoph Hellwig [not found] ` <20140924161417.GA1978-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> 2014-09-24 17:10 ` Jan Kara [not found] ` <20140924171020.GF27000-+0h/O2h83AeN3ZZ/Hiejyg@public.gmane.org> 2014-09-24 17:13 ` Christoph Hellwig [not found] ` <20140924171306.GA23874-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> 2014-09-24 17:37 ` Jan Kara 2014-09-30 20:07 ` Jan Kara 2014-09-24 14:04 ` [PATCH 2/4] Adds project ID support for ext4 Li Xi 2014-09-24 17:11 ` Jan Kara 2014-09-25 1:09 ` Li Xi 2014-09-24 14:04 ` [PATCH 3/4] Adds project quota " Li Xi [not found] ` <1411567470-31799-4-git-send-email-lixi-LfVdkaOWEx8@public.gmane.org> 2014-09-24 17:31 ` Jan Kara [not found] ` <20140924173123.GH27000-+0h/O2h83AeN3ZZ/Hiejyg@public.gmane.org> 2014-09-25 1:28 ` Li Xi [not found] ` <CAPTn0cB8X3DYprX-oKCu8aV5OLLfJJ2rh9YvMY7re69gi1s-LA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2014-09-25 11:55 ` Jan Kara 2014-09-24 14:04 ` [PATCH 4/4] Adds ioctl interface support for ext4 project Li Xi 2014-09-24 16:25 ` Jan Kara 2014-09-24 16:25 ` Jan Kara 2014-09-24 16:26 ` Christoph Hellwig 2014-09-24 16:26 ` Christoph Hellwig 2014-09-24 17:01 ` Jan Kara 2014-09-24 17:01 ` Jan Kara 2014-09-25 7:59 ` Dave Chinner 2014-09-25 11:34 ` lixi 2014-09-25 11:34 ` lixi [not found] ` <C31A739F-4502-4B40-9AE3-F2FE49291657-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2014-09-26 0:10 ` Dave Chinner 2014-09-26 0:10 ` Dave Chinner 2014-09-26 2:45 ` Li Xi 2014-09-26 2:45 ` Li Xi 2014-09-25 13:41 ` Theodore Ts'o [not found] ` <20140925134136.GE4592-AKGzg7BKzIDYtjvyW6yDsg@public.gmane.org> 2014-09-25 22:22 ` Dave Chinner 2014-09-25 22:22 ` Dave Chinner 2014-09-25 13:52 ` Jan Kara 2014-09-25 13:52 ` Jan Kara 2014-09-25 22:42 ` Dave Chinner 2014-09-26 12:01 ` Theodore Ts'o 2014-09-26 12:01 ` Theodore Ts'o 2014-09-29 15:55 ` Jan Kara 2014-09-29 15:55 ` Jan Kara 2014-09-25 7:26 ` Dave Chinner [not found] ` <1411567470-31799-5-git-send-email-lixi-LfVdkaOWEx8@public.gmane.org> 2014-09-24 16:43 ` Jan Kara
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.