Re: [PATCH 7/7][TAKE5] ext4: support new modes
On Wed, Jun 27, 2007 at 10:04:56AM +1000, David Chinner wrote: On Wed, Jun 27, 2007 at 12:59:08AM +0530, Amit K. Arora wrote: On Tue, Jun 26, 2007 at 12:14:00PM -0400, Andreas Dilger wrote: On Jun 26, 2007 17:37 +0530, Amit K. Arora wrote: 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. I'm only being the advocate for requirements David Chinner has put forward due to existing behaviour in XFS. This is one of the reasons why I think the flags mechanism we now have - we can encode the various different behaviours in any way we want and leave it to the caller. I understand. May be we can confirm once more with David Chinner if this is really required. Will it really be a compatibility issue if new XFS preallocations (ie. via fallocate) update mtime/ctime? It should be left up to the filesystem to decide. Only the filesystem knows whether something changed and the timestamp should or should not be updated. Since Andreas had suggested FA_FL_NO_MTIME flag thinking it as a requirement from XFS (whereas XFS does not need this flag), I don't think we need to add this new flag. Please let know if someone still feels FA_FL_NO_MTIME flag can be useful. -- Regards, Amit Arora - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
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: @@ -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 - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7][TAKE5] ext4: support new modes
On Jun 26, 2007 17:37 +0530, Amit K. Arora wrote: 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). That is up to the filesystem to determine then. I just thought it should be clear to return an error for flags (or as you say combinations thereof) that the filesystem doesn't understand. That said, I'd think in most cases the flags are orthogonal, so if you support some combination of the flags (e.g. FA_FL_DEL_DATA, FA_FL_DEALLOC) then you will also support other combinations of those flags just from the way it is coded. 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 ? One reason is that XFS does NOT update the mtime/ctime when doing the XFS_IOC_* allocation ioctls. 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. I'm only being the advocate for requirements David Chinner has put forward due to existing behaviour in XFS. This is one of the reasons why I think the flags mechanism we now have - we can encode the various different behaviours in any way we want and leave it to the caller. Cheers, Andreas -- Andreas Dilger Principal Software Engineer Cluster File Systems, Inc. - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7][TAKE5] ext4: support new modes
On Wed, Jun 27, 2007 at 12:59:08AM +0530, Amit K. Arora wrote: On Tue, Jun 26, 2007 at 12:14:00PM -0400, Andreas Dilger wrote: On Jun 26, 2007 17:37 +0530, Amit K. Arora wrote: 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 ? One reason is that XFS does NOT update the mtime/ctime when doing the XFS_IOC_* allocation ioctls. Not totally correct. XFS_IOC_ALLOCSP/FREESP change timestamps if they change the file size (via the truncate call made to change the file size). If they don't change the file size, then they are a no-op and should not change the file size. XFS_IOC_RESVSP/UNRESVSP don't change timestamps just like they don't change file size. That is by design AFAICT so these calls can be used by HSM-type applications that don't want to change timestamps when punching out data blocks or preallocating new ones. Hmm.. I personally will call it a bug in XFS code then. :) No, I'd call it useful. :) 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. I'm only being the advocate for requirements David Chinner has put forward due to existing behaviour in XFS. This is one of the reasons why I think the flags mechanism we now have - we can encode the various different behaviours in any way we want and leave it to the caller. I understand. May be we can confirm once more with David Chinner if this is really required. Will it really be a compatibility issue if new XFS preallocations (ie. via fallocate) update mtime/ctime? It should be left up to the filesystem to decide. Only the filesystem knows whether something changed and the timestamp should or should not be updated. Cheers, Dave. -- Dave Chinner Principal Engineer SGI Australian Software Group - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7][TAKE5] ext4: support new modes
On Jun 25, 2007 19:20 +0530, Amit K. Arora wrote: @@ -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). 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? Cheers, Andreas -- Andreas Dilger Principal Software Engineer Cluster File Systems, Inc. - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
