| Message ID | 20171011200603.27442-18-jack@suse.cz (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Wed, Oct 11, 2017 at 1:06 PM, Jan Kara <jack@suse.cz> wrote: > Now when everything is prepared, add support in ext4 to accept MAP_SYNC > as an mmap(2) flag. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/ext4/file.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/fs/ext4/file.c b/fs/ext4/file.c > index 61a8788168f3..f013cda84b3d 100644 > --- a/fs/ext4/file.c > +++ b/fs/ext4/file.c > @@ -364,6 +364,25 @@ static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma) > return 0; > } > > +#define EXT4_SUPPORTED_MAP_FLAGS (LEGACY_MAP_MASK | MAP_SYNC) > + > +static int ext4_file_mmap_validate(struct file *file, > + struct vm_area_struct *vma, > + unsigned long map_flags) > +{ > + if (map_flags & ~EXT4_SUPPORTED_MAP_FLAGS) > + return -EOPNOTSUPP; > + > + /* > + * We don't support synchronous mappings for non-DAX files. At least > + * until someone comes with a sensible use case. > + */ > + if (!IS_DAX(file_inode(file)) && (map_flags & MAP_SYNC)) > + return -EOPNOTSUPP; Perhaps EPERM instead to differentiate the unsupported flags case? There's precedent for mmap returning EPERM for reasons other than incompatible PROT flags. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 11-10-17 15:11:21, Dan Williams wrote: > On Wed, Oct 11, 2017 at 1:06 PM, Jan Kara <jack@suse.cz> wrote: > > Now when everything is prepared, add support in ext4 to accept MAP_SYNC > > as an mmap(2) flag. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/ext4/file.c | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/fs/ext4/file.c b/fs/ext4/file.c > > index 61a8788168f3..f013cda84b3d 100644 > > --- a/fs/ext4/file.c > > +++ b/fs/ext4/file.c > > @@ -364,6 +364,25 @@ static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma) > > return 0; > > } > > > > +#define EXT4_SUPPORTED_MAP_FLAGS (LEGACY_MAP_MASK | MAP_SYNC) > > + > > +static int ext4_file_mmap_validate(struct file *file, > > + struct vm_area_struct *vma, > > + unsigned long map_flags) > > +{ > > + if (map_flags & ~EXT4_SUPPORTED_MAP_FLAGS) > > + return -EOPNOTSUPP; > > + > > + /* > > + * We don't support synchronous mappings for non-DAX files. At least > > + * until someone comes with a sensible use case. > > + */ > > + if (!IS_DAX(file_inode(file)) && (map_flags & MAP_SYNC)) > > + return -EOPNOTSUPP; > > Perhaps EPERM instead to differentiate the unsupported flags case? > There's precedent for mmap returning EPERM for reasons other than > incompatible PROT flags. Hum, I could make it EINVAL. EPERM looks just too bogus to me. Honza
On Thu, Oct 12, 2017 at 6:42 AM, Jan Kara <jack@suse.cz> wrote: > On Wed 11-10-17 15:11:21, Dan Williams wrote: >> On Wed, Oct 11, 2017 at 1:06 PM, Jan Kara <jack@suse.cz> wrote: >> > Now when everything is prepared, add support in ext4 to accept MAP_SYNC >> > as an mmap(2) flag. >> > >> > Signed-off-by: Jan Kara <jack@suse.cz> >> > --- >> > fs/ext4/file.c | 20 ++++++++++++++++++++ >> > 1 file changed, 20 insertions(+) >> > >> > diff --git a/fs/ext4/file.c b/fs/ext4/file.c >> > index 61a8788168f3..f013cda84b3d 100644 >> > --- a/fs/ext4/file.c >> > +++ b/fs/ext4/file.c >> > @@ -364,6 +364,25 @@ static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma) >> > return 0; >> > } >> > >> > +#define EXT4_SUPPORTED_MAP_FLAGS (LEGACY_MAP_MASK | MAP_SYNC) >> > + >> > +static int ext4_file_mmap_validate(struct file *file, >> > + struct vm_area_struct *vma, >> > + unsigned long map_flags) >> > +{ >> > + if (map_flags & ~EXT4_SUPPORTED_MAP_FLAGS) >> > + return -EOPNOTSUPP; >> > + >> > + /* >> > + * We don't support synchronous mappings for non-DAX files. At least >> > + * until someone comes with a sensible use case. >> > + */ >> > + if (!IS_DAX(file_inode(file)) && (map_flags & MAP_SYNC)) >> > + return -EOPNOTSUPP; >> >> Perhaps EPERM instead to differentiate the unsupported flags case? >> There's precedent for mmap returning EPERM for reasons other than >> incompatible PROT flags. > > Hum, I could make it EINVAL. EPERM looks just too bogus to me. EINVAL is indistinguishable from a kernel that does not support MAP_SHARED_VALIDATE... -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
This really should be part of the previous patch. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 11, 2017 at 03:11:21PM -0700, Dan Williams wrote: > > + /* > > + * We don't support synchronous mappings for non-DAX files. At least > > + * until someone comes with a sensible use case. > > + */ > > + if (!IS_DAX(file_inode(file)) && (map_flags & MAP_SYNC)) > > + return -EOPNOTSUPP; > > Perhaps EPERM instead to differentiate the unsupported flags case? > There's precedent for mmap returning EPERM for reasons other than > incompatible PROT flags. Why would we want to voluntarily use arcane errors for a entirely new interface under our control? -EOPNOTSUPP is nice and explicit about what is going on. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 13, 2017 at 12:22 AM, Christoph Hellwig <hch@infradead.org> wrote: > On Wed, Oct 11, 2017 at 03:11:21PM -0700, Dan Williams wrote: >> > + /* >> > + * We don't support synchronous mappings for non-DAX files. At least >> > + * until someone comes with a sensible use case. >> > + */ >> > + if (!IS_DAX(file_inode(file)) && (map_flags & MAP_SYNC)) >> > + return -EOPNOTSUPP; >> >> Perhaps EPERM instead to differentiate the unsupported flags case? >> There's precedent for mmap returning EPERM for reasons other than >> incompatible PROT flags. > > Why would we want to voluntarily use arcane errors for a entirely > new interface under our control? > > -EOPNOTSUPP is nice and explicit about what is going on. We have this general and perennial problem of parsing why the kernel is throwing an error. It saves a debug step if the error code is unique, and in this case would indicate that the filesystem supports MAP_SYNC but the administrator failed to arrange for the device to be in DAX mode. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri 13-10-17 00:21:23, Christoph Hellwig wrote:
> This really should be part of the previous patch.
Fair enough, I'll fold it.
Honza
On Fri 13-10-17 08:52:28, Dan Williams wrote: > On Fri, Oct 13, 2017 at 12:22 AM, Christoph Hellwig <hch@infradead.org> wrote: > > On Wed, Oct 11, 2017 at 03:11:21PM -0700, Dan Williams wrote: > >> > + /* > >> > + * We don't support synchronous mappings for non-DAX files. At least > >> > + * until someone comes with a sensible use case. > >> > + */ > >> > + if (!IS_DAX(file_inode(file)) && (map_flags & MAP_SYNC)) > >> > + return -EOPNOTSUPP; > >> > >> Perhaps EPERM instead to differentiate the unsupported flags case? > >> There's precedent for mmap returning EPERM for reasons other than > >> incompatible PROT flags. > > > > Why would we want to voluntarily use arcane errors for a entirely > > new interface under our control? > > > > -EOPNOTSUPP is nice and explicit about what is going on. > > We have this general and perennial problem of parsing why the kernel > is throwing an error. It saves a debug step if the error code is > unique, and in this case would indicate that the filesystem supports > MAP_SYNC but the administrator failed to arrange for the device to be > in DAX mode. So I understand this wish however I think the error codes should also reflect the nature of the problem. And EPERM has something to do with access permissions, not whether file supports DAX access or not. Honza
diff --git a/fs/ext4/file.c b/fs/ext4/file.c index 61a8788168f3..f013cda84b3d 100644 --- a/fs/ext4/file.c +++ b/fs/ext4/file.c @@ -364,6 +364,25 @@ static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma) return 0; } +#define EXT4_SUPPORTED_MAP_FLAGS (LEGACY_MAP_MASK | MAP_SYNC) + +static int ext4_file_mmap_validate(struct file *file, + struct vm_area_struct *vma, + unsigned long map_flags) +{ + if (map_flags & ~EXT4_SUPPORTED_MAP_FLAGS) + return -EOPNOTSUPP; + + /* + * We don't support synchronous mappings for non-DAX files. At least + * until someone comes with a sensible use case. + */ + if (!IS_DAX(file_inode(file)) && (map_flags & MAP_SYNC)) + return -EOPNOTSUPP; + + return ext4_file_mmap(file, vma); +} + static int ext4_file_open(struct inode * inode, struct file * filp) { struct super_block *sb = inode->i_sb; @@ -723,6 +742,7 @@ const struct file_operations ext4_file_operations = { .compat_ioctl = ext4_compat_ioctl, #endif .mmap = ext4_file_mmap, + .mmap_validate = ext4_file_mmap_validate, .open = ext4_file_open, .release = ext4_release_file, .fsync = ext4_sync_file,
Now when everything is prepared, add support in ext4 to accept MAP_SYNC as an mmap(2) flag. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/ext4/file.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)