lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20131009224658.GO4446@dastard> Date: Thu, 10 Oct 2013 09:46:58 +1100 From: Dave Chinner <david@...morbit.com> To: Namjae Jeon <linkinjeon@...il.com> Cc: viro@...iv.linux.org.uk, mtk.manpages@...il.com, tytso@....edu, adilger.kernel@...ger.ca, bpm@....com, elder@...nel.org, hch@...radead.org, linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, linux-ext4@...r.kernel.org, xfs@....sgi.com, a.sangwan@...sung.com, Namjae Jeon <namjae.jeon@...sung.com> Subject: Re: [PATCH RESEND 1/7] fs: add new flag(FALLOC_FL_COLLAPSE_RANGE) for fallocate On Mon, Oct 07, 2013 at 05:12:46AM +0900, Namjae Jeon wrote: > From: Namjae Jeon <namjae.jeon@...sung.com> > > Add new flag(FALLOC_FL_COLLAPSE_RANGE) for fallocate. > updated detailed semantics in comments. > > Signed-off-by: Namjae Jeon <namjae.jeon@...sung.com> > Signed-off-by: Ashish Sangwan <a.sangwan@...sung.com> > --- > fs/open.c | 24 +++++++++++++++++++++--- > include/uapi/linux/falloc.h | 17 +++++++++++++++++ > 2 files changed, 38 insertions(+), 3 deletions(-) > > diff --git a/fs/open.c b/fs/open.c > index 7931f76..85d243a 100644 > --- a/fs/open.c > +++ b/fs/open.c > @@ -225,12 +225,14 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len) > { > struct inode *inode = file_inode(file); > long ret; > + unsigned blksize_mask = (1 << inode->i_blkbits) - 1; > > if (offset < 0 || len <= 0) > return -EINVAL; > > /* Return error if mode is not supported */ > - if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE)) > + if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE | > + FALLOC_FL_COLLAPSE_RANGE)) > return -EOPNOTSUPP; > > /* Punch hole must have keep size set */ > @@ -241,8 +243,12 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len) > if (!(file->f_mode & FMODE_WRITE)) > return -EBADF; > > - /* It's not possible punch hole on append only file */ > - if (mode & FALLOC_FL_PUNCH_HOLE && IS_APPEND(inode)) > + /* > + * It's not possible to punch hole or perform collapse range > + * on append only file > + */ > + if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_COLLAPSE_RANGE) > + && IS_APPEND(inode)) > return -EPERM; > > if (IS_IMMUTABLE(inode)) > @@ -270,6 +276,18 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len) > if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0)) > return -EFBIG; > > + /* > + * Collapse range works only on fs block size aligned offsets. > + * Check if collapse range is contained within (aligned)i_size. > + * Collapse range can only be used exclusively. > + */ > + if ((mode & FALLOC_FL_COLLAPSE_RANGE) && > + (offset & blksize_mask || len & blksize_mask || > + mode & ~FALLOC_FL_COLLAPSE_RANGE || > + (offset + len > > + round_up(i_size_read(inode), (blksize_mask + 1))))) > + return -EINVAL; There's lots of individual checks here. Let's separate them out logically. Firstly, "Collapse range can only be used exclusively" is a mode parameter check, and so should be done directly after validating the mode only contains known commands. i.e. in the first hunk above. Secondly, "Collapse range works only on fs block size aligned offsets" is an implementation constraint, not an API constraint. i.e. There is no reason why a filesystem can't implement byte range granularity for this operation, it just may not be efficient for all fielsystems and so they don't choose to implement byte range granularity. Further, filesystems might have different allocation constraints than the filesystem block size (think bigalloc on ext4, per-file extent size hints for XFS), and these generally aren't reflected in inode->i_blkbits. In these cases, the granularity of the collapse operation can only be determined by the filesystem itself, not this high level code. Hence I think the granularity check should be put into a helper function that the filesystem's ->fallocate() method calls if it can only support fs block aligned operations. That allows each filesystem to determine it's own constraints on a per-operation basis. All that remains here is the "within file size" check, and that doesn't need to be rounded up to block size to check if it is valid. If the range given overlaps the end of file in any way, then it is a truncate operation.... > diff --git a/include/uapi/linux/falloc.h b/include/uapi/linux/falloc.h > index 990c4cc..9614b72 100644 > --- a/include/uapi/linux/falloc.h > +++ b/include/uapi/linux/falloc.h > @@ -4,6 +4,23 @@ > #define FALLOC_FL_KEEP_SIZE 0x01 /* default is extend size */ > #define FALLOC_FL_PUNCH_HOLE 0x02 /* de-allocates range */ > #define FALLOC_FL_NO_HIDE_STALE 0x04 /* reserved codepoint */ > +/* > + * FALLOC_FL_COLLAPSE_RANGE: > + * This flag works in 2 steps. > + * Firstly, it deallocates any data blocks present between [offset, offset+len) > + * This step is same as punch hole and leaves a hole in the place from where > + * the blocks are removed. > + * Next, it eliminates the hole created by moving data blocks into it. > + * For extent based file systems, we achieve this functionality simply by > + * updating the starting logical offset of each extent which appears beyond > + * the hole. As this flag works on blocks of filesystem, the offset and len > + * provided to fallocate should be aligned with block size of filesystem. Hmmm - you're describing an implementation, not the API. i.e. what you need to describe is the functionality users are provided with by the flag and it's usage constraints, not how filesystems need to implement it. Something like: "FALLOC_FL_COLLAPSE_RANGE is used to remove a range of a file without leaving a hole in the file. The contents of the file beyond the range being removed is appended to the start offset of the range being removed (i.e. the hole that was punched is "collapsed"), resulting in a file layout that looks like the range that was removed never existed. As suchm collapsing a range of a file changes the size of the file, reducing it by the same length of the range that has been removed by the operation. Different filesystems may implement different limitations on the granularity of the operation. Most will limit operations to filesystem block size boundaries, but this boundary may be larger or smaller depending on the filesystem and/or the configuration of the filesystem or file. Attempting to collapse a range that crosses the end of the file is considered an illegal operation - just use ftruncate(2) if you need to collapse a range that crosses EOF." > +#define FALLOC_FL_COLLAPSE_RANGE 0x08 /* it does not leave a hole */ With the large descriptive comment, there is no need for the appended "/* it does not leave a hole */" comment. Cheers, Dave. -- Dave Chinner david@...morbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists