| Message ID | 20240827065123.1762168-4-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] block: remove checks for FALLOC_FL_NO_HIDE_STALE | expand |
On Tue, Aug 27, 2024 at 08:50:47AM +0200, Christoph Hellwig wrote: > The fallocate system call takes a mode argument, but that argument > contains a wild mix of exclusive modes and an optional flags. > > Replace FALLOC_FL_SUPPORTED_MASK with FALLOC_FL_MODE_MASK, which excludes > the optional flag bit, so that we can use switch statement on the value > to easily enumerate the cases while getting the check for duplicate modes > for free. > > To make this (and in the future the file system implementations) more > readable also add a symbolic name for the 0 mode used to allocate blocks. > > Signed-off-by: Christoph Hellwig <hch@lst.de> For a brief moment I wondered if it were possible to set more than one mode bit but it looks like none of the implementations support that kind of wackiness (e.g. COLLAPSE|INSERT_RANGE for magicks!) so we're good: Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/open.c | 51 ++++++++++++++++++------------------- > include/linux/falloc.h | 18 ++++++++----- > include/uapi/linux/falloc.h | 1 + > 3 files changed, 38 insertions(+), 32 deletions(-) > > diff --git a/fs/open.c b/fs/open.c > index 22adbef7ecc2a6..daf1b55ca8180b 100644 > --- a/fs/open.c > +++ b/fs/open.c > @@ -252,40 +252,39 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len) > if (offset < 0 || len <= 0) > return -EINVAL; > > - /* Return error if mode is not supported */ > - if (mode & ~FALLOC_FL_SUPPORTED_MASK) > + if (mode & ~(FALLOC_FL_MODE_MASK | FALLOC_FL_KEEP_SIZE)) > return -EOPNOTSUPP; > > - /* Punch hole and zero range are mutually exclusive */ > - if ((mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE)) == > - (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE)) > - return -EOPNOTSUPP; > - > - /* Punch hole must have keep size set */ > - if ((mode & FALLOC_FL_PUNCH_HOLE) && > - !(mode & FALLOC_FL_KEEP_SIZE)) > + /* > + * Modes are exclusive, even if that is not obvious from the encoding > + * as bit masks and the mix with the flag in the same namespace. > + * > + * To make things even more complicated, FALLOC_FL_ALLOCATE_RANGE is > + * encoded as no bit set. > + */ > + switch (mode & FALLOC_FL_MODE_MASK) { > + case FALLOC_FL_ALLOCATE_RANGE: > + case FALLOC_FL_UNSHARE_RANGE: > + case FALLOC_FL_ZERO_RANGE: > + break; > + case FALLOC_FL_PUNCH_HOLE: > + if (!(mode & FALLOC_FL_KEEP_SIZE)) > + return -EOPNOTSUPP; > + break; > + case FALLOC_FL_COLLAPSE_RANGE: > + case FALLOC_FL_INSERT_RANGE: > + if (mode & FALLOC_FL_KEEP_SIZE) > + return -EOPNOTSUPP; > + break; > + default: > return -EOPNOTSUPP; > - > - /* Collapse range should only be used exclusively. */ > - if ((mode & FALLOC_FL_COLLAPSE_RANGE) && > - (mode & ~FALLOC_FL_COLLAPSE_RANGE)) > - return -EINVAL; > - > - /* Insert range should only be used exclusively. */ > - if ((mode & FALLOC_FL_INSERT_RANGE) && > - (mode & ~FALLOC_FL_INSERT_RANGE)) > - return -EINVAL; > - > - /* Unshare range should only be used with allocate mode. */ > - if ((mode & FALLOC_FL_UNSHARE_RANGE) && > - (mode & ~(FALLOC_FL_UNSHARE_RANGE | FALLOC_FL_KEEP_SIZE))) > - return -EINVAL; > + } > > if (!(file->f_mode & FMODE_WRITE)) > return -EBADF; > > /* > - * We can only allow pure fallocate on append only files > + * On append-only files only space preallocation is supported. > */ > if ((mode & ~FALLOC_FL_KEEP_SIZE) && IS_APPEND(inode)) > return -EPERM; > diff --git a/include/linux/falloc.h b/include/linux/falloc.h > index f3f0b97b167579..3f49f3df6af5fb 100644 > --- a/include/linux/falloc.h > +++ b/include/linux/falloc.h > @@ -25,12 +25,18 @@ struct space_resv { > #define FS_IOC_UNRESVSP64 _IOW('X', 43, struct space_resv) > #define FS_IOC_ZERO_RANGE _IOW('X', 57, struct space_resv) > > -#define FALLOC_FL_SUPPORTED_MASK (FALLOC_FL_KEEP_SIZE | \ > - FALLOC_FL_PUNCH_HOLE | \ > - FALLOC_FL_COLLAPSE_RANGE | \ > - FALLOC_FL_ZERO_RANGE | \ > - FALLOC_FL_INSERT_RANGE | \ > - FALLOC_FL_UNSHARE_RANGE) > +/* > + * Mask of all supported fallocate modes. Only one can be set at a time. > + * > + * In addition to the mode bit, the mode argument can also encode flags. > + * FALLOC_FL_KEEP_SIZE is the only supported flag so far. > + */ > +#define FALLOC_FL_MODE_MASK (FALLOC_FL_ALLOCATE_RANGE | \ > + FALLOC_FL_PUNCH_HOLE | \ > + FALLOC_FL_COLLAPSE_RANGE | \ > + FALLOC_FL_ZERO_RANGE | \ > + FALLOC_FL_INSERT_RANGE | \ > + FALLOC_FL_UNSHARE_RANGE) > > /* on ia32 l_start is on a 32-bit boundary */ > #if defined(CONFIG_X86_64) > diff --git a/include/uapi/linux/falloc.h b/include/uapi/linux/falloc.h > index 51398fa57f6cdf..5810371ed72bbd 100644 > --- a/include/uapi/linux/falloc.h > +++ b/include/uapi/linux/falloc.h > @@ -2,6 +2,7 @@ > #ifndef _UAPI_FALLOC_H_ > #define _UAPI_FALLOC_H_ > > +#define FALLOC_FL_ALLOCATE_RANGE 0x00 /* allocate range */ > #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 */ > -- > 2.43.0 > >
On Tue, Aug 27, 2024 at 07:55:02AM -0700, Darrick J. Wong wrote: > For a brief moment I wondered if it were possible to set more than one > mode bit but it looks like none of the implementations support that kind > of wackiness (e.g. COLLAPSE|INSERT_RANGE for magicks!) so we're good: Yes, we check for all possible mode combinations and reject them already, just in had to read an not extensible way.
On Tue 27-08-24 08:50:47, Christoph Hellwig wrote: > The fallocate system call takes a mode argument, but that argument > contains a wild mix of exclusive modes and an optional flags. > > Replace FALLOC_FL_SUPPORTED_MASK with FALLOC_FL_MODE_MASK, which excludes > the optional flag bit, so that we can use switch statement on the value > to easily enumerate the cases while getting the check for duplicate modes > for free. > > To make this (and in the future the file system implementations) more > readable also add a symbolic name for the 0 mode used to allocate blocks. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Looks good. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/open.c | 51 ++++++++++++++++++------------------- > include/linux/falloc.h | 18 ++++++++----- > include/uapi/linux/falloc.h | 1 + > 3 files changed, 38 insertions(+), 32 deletions(-) > > diff --git a/fs/open.c b/fs/open.c > index 22adbef7ecc2a6..daf1b55ca8180b 100644 > --- a/fs/open.c > +++ b/fs/open.c > @@ -252,40 +252,39 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len) > if (offset < 0 || len <= 0) > return -EINVAL; > > - /* Return error if mode is not supported */ > - if (mode & ~FALLOC_FL_SUPPORTED_MASK) > + if (mode & ~(FALLOC_FL_MODE_MASK | FALLOC_FL_KEEP_SIZE)) > return -EOPNOTSUPP; > > - /* Punch hole and zero range are mutually exclusive */ > - if ((mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE)) == > - (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE)) > - return -EOPNOTSUPP; > - > - /* Punch hole must have keep size set */ > - if ((mode & FALLOC_FL_PUNCH_HOLE) && > - !(mode & FALLOC_FL_KEEP_SIZE)) > + /* > + * Modes are exclusive, even if that is not obvious from the encoding > + * as bit masks and the mix with the flag in the same namespace. > + * > + * To make things even more complicated, FALLOC_FL_ALLOCATE_RANGE is > + * encoded as no bit set. > + */ > + switch (mode & FALLOC_FL_MODE_MASK) { > + case FALLOC_FL_ALLOCATE_RANGE: > + case FALLOC_FL_UNSHARE_RANGE: > + case FALLOC_FL_ZERO_RANGE: > + break; > + case FALLOC_FL_PUNCH_HOLE: > + if (!(mode & FALLOC_FL_KEEP_SIZE)) > + return -EOPNOTSUPP; > + break; > + case FALLOC_FL_COLLAPSE_RANGE: > + case FALLOC_FL_INSERT_RANGE: > + if (mode & FALLOC_FL_KEEP_SIZE) > + return -EOPNOTSUPP; > + break; > + default: > return -EOPNOTSUPP; > - > - /* Collapse range should only be used exclusively. */ > - if ((mode & FALLOC_FL_COLLAPSE_RANGE) && > - (mode & ~FALLOC_FL_COLLAPSE_RANGE)) > - return -EINVAL; > - > - /* Insert range should only be used exclusively. */ > - if ((mode & FALLOC_FL_INSERT_RANGE) && > - (mode & ~FALLOC_FL_INSERT_RANGE)) > - return -EINVAL; > - > - /* Unshare range should only be used with allocate mode. */ > - if ((mode & FALLOC_FL_UNSHARE_RANGE) && > - (mode & ~(FALLOC_FL_UNSHARE_RANGE | FALLOC_FL_KEEP_SIZE))) > - return -EINVAL; > + } > > if (!(file->f_mode & FMODE_WRITE)) > return -EBADF; > > /* > - * We can only allow pure fallocate on append only files > + * On append-only files only space preallocation is supported. > */ > if ((mode & ~FALLOC_FL_KEEP_SIZE) && IS_APPEND(inode)) > return -EPERM; > diff --git a/include/linux/falloc.h b/include/linux/falloc.h > index f3f0b97b167579..3f49f3df6af5fb 100644 > --- a/include/linux/falloc.h > +++ b/include/linux/falloc.h > @@ -25,12 +25,18 @@ struct space_resv { > #define FS_IOC_UNRESVSP64 _IOW('X', 43, struct space_resv) > #define FS_IOC_ZERO_RANGE _IOW('X', 57, struct space_resv) > > -#define FALLOC_FL_SUPPORTED_MASK (FALLOC_FL_KEEP_SIZE | \ > - FALLOC_FL_PUNCH_HOLE | \ > - FALLOC_FL_COLLAPSE_RANGE | \ > - FALLOC_FL_ZERO_RANGE | \ > - FALLOC_FL_INSERT_RANGE | \ > - FALLOC_FL_UNSHARE_RANGE) > +/* > + * Mask of all supported fallocate modes. Only one can be set at a time. > + * > + * In addition to the mode bit, the mode argument can also encode flags. > + * FALLOC_FL_KEEP_SIZE is the only supported flag so far. > + */ > +#define FALLOC_FL_MODE_MASK (FALLOC_FL_ALLOCATE_RANGE | \ > + FALLOC_FL_PUNCH_HOLE | \ > + FALLOC_FL_COLLAPSE_RANGE | \ > + FALLOC_FL_ZERO_RANGE | \ > + FALLOC_FL_INSERT_RANGE | \ > + FALLOC_FL_UNSHARE_RANGE) > > /* on ia32 l_start is on a 32-bit boundary */ > #if defined(CONFIG_X86_64) > diff --git a/include/uapi/linux/falloc.h b/include/uapi/linux/falloc.h > index 51398fa57f6cdf..5810371ed72bbd 100644 > --- a/include/uapi/linux/falloc.h > +++ b/include/uapi/linux/falloc.h > @@ -2,6 +2,7 @@ > #ifndef _UAPI_FALLOC_H_ > #define _UAPI_FALLOC_H_ > > +#define FALLOC_FL_ALLOCATE_RANGE 0x00 /* allocate range */ > #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 */ > -- > 2.43.0 >
diff --git a/fs/open.c b/fs/open.c index 22adbef7ecc2a6..daf1b55ca8180b 100644 --- a/fs/open.c +++ b/fs/open.c @@ -252,40 +252,39 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len) if (offset < 0 || len <= 0) return -EINVAL; - /* Return error if mode is not supported */ - if (mode & ~FALLOC_FL_SUPPORTED_MASK) + if (mode & ~(FALLOC_FL_MODE_MASK | FALLOC_FL_KEEP_SIZE)) return -EOPNOTSUPP; - /* Punch hole and zero range are mutually exclusive */ - if ((mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE)) == - (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE)) - return -EOPNOTSUPP; - - /* Punch hole must have keep size set */ - if ((mode & FALLOC_FL_PUNCH_HOLE) && - !(mode & FALLOC_FL_KEEP_SIZE)) + /* + * Modes are exclusive, even if that is not obvious from the encoding + * as bit masks and the mix with the flag in the same namespace. + * + * To make things even more complicated, FALLOC_FL_ALLOCATE_RANGE is + * encoded as no bit set. + */ + switch (mode & FALLOC_FL_MODE_MASK) { + case FALLOC_FL_ALLOCATE_RANGE: + case FALLOC_FL_UNSHARE_RANGE: + case FALLOC_FL_ZERO_RANGE: + break; + case FALLOC_FL_PUNCH_HOLE: + if (!(mode & FALLOC_FL_KEEP_SIZE)) + return -EOPNOTSUPP; + break; + case FALLOC_FL_COLLAPSE_RANGE: + case FALLOC_FL_INSERT_RANGE: + if (mode & FALLOC_FL_KEEP_SIZE) + return -EOPNOTSUPP; + break; + default: return -EOPNOTSUPP; - - /* Collapse range should only be used exclusively. */ - if ((mode & FALLOC_FL_COLLAPSE_RANGE) && - (mode & ~FALLOC_FL_COLLAPSE_RANGE)) - return -EINVAL; - - /* Insert range should only be used exclusively. */ - if ((mode & FALLOC_FL_INSERT_RANGE) && - (mode & ~FALLOC_FL_INSERT_RANGE)) - return -EINVAL; - - /* Unshare range should only be used with allocate mode. */ - if ((mode & FALLOC_FL_UNSHARE_RANGE) && - (mode & ~(FALLOC_FL_UNSHARE_RANGE | FALLOC_FL_KEEP_SIZE))) - return -EINVAL; + } if (!(file->f_mode & FMODE_WRITE)) return -EBADF; /* - * We can only allow pure fallocate on append only files + * On append-only files only space preallocation is supported. */ if ((mode & ~FALLOC_FL_KEEP_SIZE) && IS_APPEND(inode)) return -EPERM; diff --git a/include/linux/falloc.h b/include/linux/falloc.h index f3f0b97b167579..3f49f3df6af5fb 100644 --- a/include/linux/falloc.h +++ b/include/linux/falloc.h @@ -25,12 +25,18 @@ struct space_resv { #define FS_IOC_UNRESVSP64 _IOW('X', 43, struct space_resv) #define FS_IOC_ZERO_RANGE _IOW('X', 57, struct space_resv) -#define FALLOC_FL_SUPPORTED_MASK (FALLOC_FL_KEEP_SIZE | \ - FALLOC_FL_PUNCH_HOLE | \ - FALLOC_FL_COLLAPSE_RANGE | \ - FALLOC_FL_ZERO_RANGE | \ - FALLOC_FL_INSERT_RANGE | \ - FALLOC_FL_UNSHARE_RANGE) +/* + * Mask of all supported fallocate modes. Only one can be set at a time. + * + * In addition to the mode bit, the mode argument can also encode flags. + * FALLOC_FL_KEEP_SIZE is the only supported flag so far. + */ +#define FALLOC_FL_MODE_MASK (FALLOC_FL_ALLOCATE_RANGE | \ + FALLOC_FL_PUNCH_HOLE | \ + FALLOC_FL_COLLAPSE_RANGE | \ + FALLOC_FL_ZERO_RANGE | \ + FALLOC_FL_INSERT_RANGE | \ + FALLOC_FL_UNSHARE_RANGE) /* on ia32 l_start is on a 32-bit boundary */ #if defined(CONFIG_X86_64) diff --git a/include/uapi/linux/falloc.h b/include/uapi/linux/falloc.h index 51398fa57f6cdf..5810371ed72bbd 100644 --- a/include/uapi/linux/falloc.h +++ b/include/uapi/linux/falloc.h @@ -2,6 +2,7 @@ #ifndef _UAPI_FALLOC_H_ #define _UAPI_FALLOC_H_ +#define FALLOC_FL_ALLOCATE_RANGE 0x00 /* allocate range */ #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 */
The fallocate system call takes a mode argument, but that argument contains a wild mix of exclusive modes and an optional flags. Replace FALLOC_FL_SUPPORTED_MASK with FALLOC_FL_MODE_MASK, which excludes the optional flag bit, so that we can use switch statement on the value to easily enumerate the cases while getting the check for duplicate modes for free. To make this (and in the future the file system implementations) more readable also add a symbolic name for the 0 mode used to allocate blocks. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/open.c | 51 ++++++++++++++++++------------------- include/linux/falloc.h | 18 ++++++++----- include/uapi/linux/falloc.h | 1 + 3 files changed, 38 insertions(+), 32 deletions(-)