On Fri, Jan 09, 2015 at 10:47:58AM +0100, Jan Kara wrote:

On Thu 08-01-15 15:20:21, Andreas Dilger wrote:

quoted

On Jan 8, 2015, at 1:26 AM, Jan Kara [off-list ref] wrote:

quoted

On Tue 09-12-14 13:22:25, Li Xi wrote:

quoted

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.

 I have noticed one thing you apparently changed in v7 of the patch set.
See below.

quoted

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 29c43e7..8bd1da9 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h

@@ -377,16 +377,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		FS_PROJINHERIT_FL /* Create with parents projid */

 How did FS_PROJINHERIT_FL get here? There used to be 0x20000000 in older
version of the patch set which is correct - this definition is defining
ext4 on-disk format. As such it is an ext4 specific flag and should be
definined to a fixed constant independed of any other filesystem. It seems
you are somewhat mixing what is an on-disk format flag value and what is a
flag value passed from userspace. These two may be different things and
you need to convert between the values when getting / setting flags...

Currently the EXT4_*_FL and FS_*_FL values are all identical, and there
is no reason to change that before it is actually needed.  Since the
FS_PROJINHERIT_FL is used via chattr/lsattr from userspace, this value
must also be kept the same in the future to avoid API breakage, so there
is no reason to worry about incompatibilities.

  Agreed. I was somewhat worried about having on-disk flag defined through
the external non-ext4 define but you are right that neither can really
change once we ship a kernel with it.

quoted

See also the [v8 5/5] patch, which is changing the EXT4_*_FL values to
use FS_*_FL constants, where applicable, so that it is more clear that
these values need to be the same.

  OK, I've missed that. So if things will be consistent again, I'm fine
with the change.

Except that I NACK'd that change (i.e patch 4/5) because it's out of
scope of a "support project quota" patchset. not to mention that it
is broken because it exhausts the flags space with ext4 specific
flags and prevents future expansion of the ioctl structure.

Any extension to the ioctl needs to be done in a spearate patch set,
with separate justification. This patch set should only implement
the very minimum needed to use the project quota ioctl flags....

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org