Re: [PATCH 0/3] Data in direntry (dirdata) feature

["Theodore Tso" <tytso@mit.edu>]

Hi Artem,

Thank you for sending this patch.  My primary concern with it is that
it seems to be optimized for minimizing the changes for the
out-of-tree Lustre Server patches, but not for necessasrily from the
upstream perspective.

First, there is no way to actually set any dirdata feature using just
the upstream patch.  Presumably, the out-of-tree Lustre patches call
ext4_insert_dentry_data() with a non-NULL data, or add_dirent_to_buf()
in fs/ext4/namei.c was further patched so that the data variable on
the stack gets set to a non-NULL --- but in your patches, data is
always NULL, which manes the data passed into
ext4_insert_dentry_data() is always NULL.

Since there is no way to actually set dirdata after the upstream
kernel, nor is there any way to set the dirdata using debugfs (the
patches to e2fsprogs only affect e2fsck, and there is no way to
examine the dirdata or set dirdata via debugfs), it's going to make it
very hard to test the kernel patches without using pre-existing file
system images.  I haven't seen the xfstests patches from you yet, but
this is the only way you could test the code paths as far as I can
tell.

Secondly, there are a lot of changes because you rename functions like
ext4_dir_rec_len() to ext4_dirent_rec_len() and ext4_insert_dirent()
to ext4_insert_dentry_data().  Especially for the ext4_dir_rec_len()
change, it's not strictly speaking necessary, and while I can see the
argument that it might make it more readable / understandable, if you
*really* want to do the rename, it's better to break that out into a
separate patch which *just* renames the function, and then change the
function prototype in a subsequent patch.


As far as how get this patch upstream, the challenge with LUFID (and
dirdata in general), is that the dirdata has to set when the directory
entry is created, and is immutable afterwards.  And so if we want to
have a way to create directory entries with an LUFID, we either need
to (a) use the out-of-tree Lustre patches, (b) plumb through some new
interface through the VFS and potentially some new openat3() system
call, or (c) add some new ext4 ioctl EXT4_IOC_SET_LUFID which sets the
LUFID on an already existing pathname.

(c) is problematic since it's a different code path than what the
Lustre server would use, and it would require using the dentry used to
opened the file to decide which directory entry to mutate, and it also
violates the presumption that the directory entry's dirdata is
immutable after the directory entry is created.

Speaking of which, I'm really confused with *why* the LUFID is being
stored in the diretory entry.  From what I can tell, from [1], the FID
is essentially a distributed inode number.  Given that, why isn't the
LUFID stored in an xattr?  What am I missing?

[1] https://wiki.lustre.org/Understanding_Lustre_Internals

						- Ted