From: Jan Kara Subject: Re: [PATCH 3/4] Adds project quota support for ext4 Date: Thu, 25 Sep 2014 13:55:31 +0200 Message-ID: <20140925115531.GA15352@quack.suse.cz> References: <1411567470-31799-1-git-send-email-lixi@ddn.com> <1411567470-31799-4-git-send-email-lixi@ddn.com> <20140924173123.GH27000@quack.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Jan Kara , "linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Ext4 Developers List , "linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Theodore Ts'o , Andreas Dilger , "viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org" , "hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org" , Dmitry Monakhov To: Li Xi Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-ext4.vger.kernel.org On Thu 25-09-14 09:28:24, Li Xi wrote: > On Thu, Sep 25, 2014 at 1:31 AM, Jan Kara 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 SUSE Labs, CR