Re: [PATCH 7/7][TAKE5] ext4: support new modes

On Mon, Jun 25, 2007 at 03:56:25PM -0600, Andreas Dilger wrote:

On Jun 25, 2007  19:20 +0530, Amit K. Arora wrote:

quoted

@@ -2499,7 +2500,8 @@ long ext4_fallocate(struct inode *inode,
 	 * currently supporting (pre)allocate mode for extent-based
 	 * files _only_
 	 */
-	if (mode != FA_ALLOCATE || !(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL))
+	if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL) ||
+		!(mode == FA_ALLOCATE || mode == FA_RESV_SPACE))
 		return -EOPNOTSUPP;

This should probably just check for the individual flags it can support
(e.g. no FA_FL_DEALLOC, no FA_FL_DEL_DATA).

Hmm.. I am thinking of a scenario when the file system supports some
individual flags, but does not support a particular combination of them.
Just for example sake, assume we have FA_ZERO_SPACE mode also. Now, if a
file system supports FA_ZERO_SPACE, FA_ALLOCATE, FA_DEALLOCATE and
FA_RESV_SPACE; and no other mode (i.e. FA_UNRESV_SPACE is not supported
for some reason). This means that although we support FA_FL_DEALLOC,
FA_FL_KEEP_SIZE and FA_FL_DEL_DATA flags, but we do not support the
combination of all these flags (which is nothing but FA_UNRESV_SPACE).
 

I also thought another proposed flag was to determine whether mtime (and
maybe ctime) is changed when doing prealloc/dealloc space?  Default should
probably be to change mtime/ctime, and have FA_FL_NO_MTIME.  Someone else
should decide if we want to allow changing the file w/o changing ctime, if
that is required even though the file is not visibly changing.  Maybe the
ctime update should be implicit if the size or mtime are changing?

Is it really required ? I mean, why should we allow users not to update
ctime/mtime even if the file metadata/data gets updated ? It sounds
a bit "unnatural" to me.
Is there any application scenario in your mind, when you suggest of
giving this flexibility to userspace ?

I think, modifying ctime/mtime should be dependent on the other flags.
E.g., if we do not zero out data blocks on allocation/deallocation,
update only ctime. Otherwise, update ctime and mtime both.

--
Regards,
Amit Arora