Re: [PATCH V3 7/8] fs/ext4: Introduce DAX inode flag

[Andreas Dilger]

On May 20, 2020, at 2:55 PM, Darrick J. Wong <darrick.wong@xxxxxxxxxx> wrote:
> On Wed, May 20, 2020 at 01:02:42PM -0700, Ira Weiny wrote:
>> On Wed, May 20, 2020 at 01:26:44PM -0600, Andreas Dilger wrote:
>>> On May 19, 2020, at 11:57 PM, ira.weiny@xxxxxxxxx wrote:
>>>> 
>>>> From: Ira Weiny <ira.weiny@xxxxxxxxx>
>>>> 
>>>> Add a flag to preserve FS_XFLAG_DAX in the ext4 inode.
>>>> 
>>>> Set the flag to be user visible and changeable.  Set the flag to be
>>>> inherited.  Allow applications to change the flag at any time with the
>>>> exception of if VERITY or ENCRYPT is set.
>>>> 
>>>> Disallow setting VERITY or ENCRYPT if DAX is set.
>>>> 
>>>> Finally, on regular files, flag the inode to not be cached to facilitate
>>>> changing S_DAX on the next creation of the inode.
>>>> 
>>>> Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
>>>> 
>>>> ---
>>>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>>>> index 6235440e4c39..467c30a789b6 100644
>>>> --- a/fs/ext4/ext4.h
>>>> +++ b/fs/ext4/ext4.h
>>>> @@ -415,13 +415,16 @@ struct flex_groups {
>>>> #define EXT4_VERITY_FL			0x00100000 /* Verity protected inode */
>>>> #define EXT4_EA_INODE_FL	        0x00200000 /* Inode used for large EA */
>>>> /* 0x00400000 was formerly EXT4_EOFBLOCKS_FL */
>>>> +
>>>> +#define EXT4_DAX_FL			0x01000000 /* Inode is DAX */
>>>> +
>>>> #define EXT4_INLINE_DATA_FL		0x10000000 /* Inode has inline data. */
>>>> #define EXT4_PROJINHERIT_FL		0x20000000 /* Create with parents projid */
>>>> #define EXT4_CASEFOLD_FL		0x40000000 /* Casefolded file */
>>>> #define EXT4_RESERVED_FL		0x80000000 /* reserved for ext4 lib */
>>> 
>>> Hi Ira,
>>> This flag value conflicts with the reserved flag in e2fsprogs for snapshots:
>>> 
>>> #define EXT4_SNAPFILE_FL                0x01000000  /* Inode is a snapshot */
>> 
>> Sure NP but is that new?  I'm building off of 5.7-rc4.
>> 
>> Just curious if I completely missed something.
> 
> Yeah, you missed that ... for some reason the kernel ext4 driver is
> missing flags that are in e2fsprogs.  (huh??)

It's no different than ext2 not having the full set of bits defined or
in use.

> I would say you could probably just take over the flag because the 2010s
> called and they don't want next3 back.  I guess that leaves 0x02000000
> as the sole unclaimed bit, but this seriously needs some cleaning.

Darrick,
we are in the process of updating the snapshot code for ext4, so need to
keep the 0x01000000 bit for snapshots.  Since 0x02000000 has never been
used for anything, there is no reason not to use it instead.

If we need to reclaim flags, it would be better to look at "COMPR" flags:

/* Reserved for compression usage... */
#define FS_COMPR_FL           0x00000004 /* Compress file */
#define FS_DIRTY_FL           0x00000100
#define FS_COMPRBLK_FL        0x00000200 /* One or more compressed clusters */
#define FS_NOCOMP_FL          0x00000400 /* Don't compress */

since I don't think they have ever been used.  I don't think we need 4x
on-disk state flags for that, especially not as part of the API.  It is
enough to have FS_COMPR_FL for the API, and then handle internal state
separately (e.g. compress into a separate on-disk extent and then swap
extents atomically instead of storing transient state on disk).

Cheers, Andreas

Attachment: signature.asc
Description: Message signed with OpenPGP