| Message ID | 20180511192651.21324-3-mfasheh@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 11, 2018 at 12:26:51PM -0700, Mark Fasheh wrote: > Right now we return EINVAL if a process does not have permission to dedupe a > file. This was an oversight on my part. EPERM gives a true description of > the nature of our error, and EINVAL is already used for the case that the > filesystem does not support dedupe. > > Signed-off-by: Mark Fasheh <mfasheh@suse.de> > --- > fs/read_write.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/read_write.c b/fs/read_write.c > index 77986a2e2a3b..8edef43a182c 100644 > --- a/fs/read_write.c > +++ b/fs/read_write.c > @@ -2038,7 +2038,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same) > info->status = -EINVAL; > } else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE) || > uid_eq(current_fsuid(), dst->i_uid))) { > - info->status = -EINVAL; > + info->status = -EPERM; Hmm, are we allowed to change this aspect of the kabi after the fact? Granted, we're only trading one error code for another, but will the existing users of this care? xfs_io won't and I assume duperemove won't either, but what about bees? :) --D > } else if (file->f_path.mnt != dst_file->f_path.mnt) { > info->status = -EXDEV; > } else if (S_ISDIR(dst->i_mode)) { > -- > 2.15.1 >
On Sat, May 12, 2018 at 3:06 AM, Darrick J. Wong <darrick.wong@oracle.com> wrote: > On Fri, May 11, 2018 at 12:26:51PM -0700, Mark Fasheh wrote: >> Right now we return EINVAL if a process does not have permission to dedupe a >> file. This was an oversight on my part. EPERM gives a true description of >> the nature of our error, and EINVAL is already used for the case that the >> filesystem does not support dedupe. >> >> Signed-off-by: Mark Fasheh <mfasheh@suse.de> >> --- >> fs/read_write.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/read_write.c b/fs/read_write.c >> index 77986a2e2a3b..8edef43a182c 100644 >> --- a/fs/read_write.c >> +++ b/fs/read_write.c >> @@ -2038,7 +2038,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same) >> info->status = -EINVAL; >> } else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE) || >> uid_eq(current_fsuid(), dst->i_uid))) { >> - info->status = -EINVAL; >> + info->status = -EPERM; > > Hmm, are we allowed to change this aspect of the kabi after the fact? > > Granted, we're only trading one error code for another, but will the > existing users of this care? xfs_io won't and I assume duperemove won't > either, but what about bees? :) > Relaxing -EINVAL is the common case with kabi. Every new flag we add support for does that and is it also common that a new flag we support is restricted for certain capabilities so EINVAL is replaced with EPERM. BTW, man page doesn't say anything about the is_admin case. IMO EPERM makes perfect sense here and btw, we also return EPERM from overlayfs if dst is on lower layer. Mark, Please be aware that these changes conflict with Miklos' dedupe-cleanup patches, so I suggest you collaborate on that https://marc.info/?l=linux-fsdevel&m=152568128031031&w=2 Thanks, Amir.
On Fri, May 11, 2018 at 05:06:34PM -0700, Darrick J. Wong wrote: > On Fri, May 11, 2018 at 12:26:51PM -0700, Mark Fasheh wrote: > > Right now we return EINVAL if a process does not have permission to dedupe a > > file. This was an oversight on my part. EPERM gives a true description of > > the nature of our error, and EINVAL is already used for the case that the > > filesystem does not support dedupe. > > - info->status = -EINVAL; > > + info->status = -EPERM; > > Hmm, are we allowed to change this aspect of the kabi after the fact? > > Granted, we're only trading one error code for another, but will the > existing users of this care? xfs_io won't and I assume duperemove won't > either, but what about bees? :) There's more: https://codesearch.debian.net/search?q=FILE_EXTENT_SAME This includes only software that has been packaged for Debian (notably, not bees), but that gives enough interesting coverage. And none of these cases discriminate between error codes -- they merely report them to the user. Thus, I can't think of a downside of making the error code more accurate. Meow!
On Fri, May 11, 2018 at 05:06:34PM -0700, Darrick J. Wong wrote: > On Fri, May 11, 2018 at 12:26:51PM -0700, Mark Fasheh wrote: > > Right now we return EINVAL if a process does not have permission to dedupe a > > file. This was an oversight on my part. EPERM gives a true description of > > the nature of our error, and EINVAL is already used for the case that the > > filesystem does not support dedupe. > > > > Signed-off-by: Mark Fasheh <mfasheh@suse.de> > > --- > > fs/read_write.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/read_write.c b/fs/read_write.c > > index 77986a2e2a3b..8edef43a182c 100644 > > --- a/fs/read_write.c > > +++ b/fs/read_write.c > > @@ -2038,7 +2038,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same) > > info->status = -EINVAL; > > } else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE) || > > uid_eq(current_fsuid(), dst->i_uid))) { > > - info->status = -EINVAL; > > + info->status = -EPERM; > > Hmm, are we allowed to change this aspect of the kabi after the fact? > > Granted, we're only trading one error code for another, but will the > existing users of this care? xfs_io won't and I assume duperemove won't > either, but what about bees? :) Yeah if you see my initial e-mail I check bees and also rust-btrfs. I think this is fine as we're simply expanding on an error code return. There's no magic behavior expected with respect to these error codes either. --Mark
On Sun, May 13, 2018 at 06:21:52PM +0000, Mark Fasheh wrote: > On Fri, May 11, 2018 at 05:06:34PM -0700, Darrick J. Wong wrote: > > On Fri, May 11, 2018 at 12:26:51PM -0700, Mark Fasheh wrote: > > > Right now we return EINVAL if a process does not have permission to dedupe a > > > file. This was an oversight on my part. EPERM gives a true description of > > > the nature of our error, and EINVAL is already used for the case that the > > > filesystem does not support dedupe. > > > > > > Signed-off-by: Mark Fasheh <mfasheh@suse.de> > > > --- > > > fs/read_write.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/read_write.c b/fs/read_write.c > > > index 77986a2e2a3b..8edef43a182c 100644 > > > --- a/fs/read_write.c > > > +++ b/fs/read_write.c > > > @@ -2038,7 +2038,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same) > > > info->status = -EINVAL; > > > } else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE) || > > > uid_eq(current_fsuid(), dst->i_uid))) { > > > - info->status = -EINVAL; > > > + info->status = -EPERM; > > > > Hmm, are we allowed to change this aspect of the kabi after the fact? > > > > Granted, we're only trading one error code for another, but will the > > existing users of this care? xfs_io won't and I assume duperemove won't > > either, but what about bees? :) > > Yeah if you see my initial e-mail I check bees and also rust-btrfs. I think > this is fine as we're simply expanding on an error code return. There's no > magic behavior expected with respect to these error codes either. Ok. No objections from me, then. Acked-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --Mark > -- > 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, May 11, 2018 at 12:26:51PM -0700, Mark Fasheh wrote: > Right now we return EINVAL if a process does not have permission to dedupe a > file. This was an oversight on my part. EPERM gives a true description of > the nature of our error, and EINVAL is already used for the case that the > filesystem does not support dedupe. > > Signed-off-by: Mark Fasheh <mfasheh@suse.de> Acked-by: David Sterba <dsterba@suse.com> We've been using EINVAL when the request is invalid in the ioctls, eg. combination of arguments that does not make sense, while EPERM is for cases when the request is ok but there's still some permission missing, > } else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE) || > uid_eq(current_fsuid(), dst->i_uid))) { > - info->status = -EINVAL; > + info->status = -EPERM; exactly like this.
On Sun, May 13, 2018 at 11:26:39AM -0700, Darrick J. Wong wrote: > On Sun, May 13, 2018 at 06:21:52PM +0000, Mark Fasheh wrote: > > On Fri, May 11, 2018 at 05:06:34PM -0700, Darrick J. Wong wrote: > > > On Fri, May 11, 2018 at 12:26:51PM -0700, Mark Fasheh wrote: > > > > Right now we return EINVAL if a process does not have permission to dedupe a > > > > file. This was an oversight on my part. EPERM gives a true description of > > > > the nature of our error, and EINVAL is already used for the case that the > > > > filesystem does not support dedupe. > > > > > > > > Signed-off-by: Mark Fasheh <mfasheh@suse.de> > > > > --- > > > > fs/read_write.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/fs/read_write.c b/fs/read_write.c > > > > index 77986a2e2a3b..8edef43a182c 100644 > > > > --- a/fs/read_write.c > > > > +++ b/fs/read_write.c > > > > @@ -2038,7 +2038,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same) > > > > info->status = -EINVAL; > > > > } else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE) || > > > > uid_eq(current_fsuid(), dst->i_uid))) { > > > > - info->status = -EINVAL; > > > > + info->status = -EPERM; > > > > > > Hmm, are we allowed to change this aspect of the kabi after the fact? > > > > > > Granted, we're only trading one error code for another, but will the > > > existing users of this care? xfs_io won't and I assume duperemove won't > > > either, but what about bees? :) > > > > Yeah if you see my initial e-mail I check bees and also rust-btrfs. I think > > this is fine as we're simply expanding on an error code return. There's no > > magic behavior expected with respect to these error codes either. > > Ok. No objections from me, then. > > Acked-by: Darrick J. Wong <darrick.wong@oracle.com> For what it's worth, no objection from me either. ;) bees runs only with admin privilege and will never hit the modified line. If bees is started without admin privilege, the TREE_SEARCH_V2 ioctl fails. bees uses this ioctl to walk over all the data in the filesystem, so without admin privilege, bees never opens, reads, or dedupes anything. bees relies on having an accurate internal model of btrfs structure and behavior to issue dedup commands that will work and do useful things; however, unexpected kernel behavior or concurrent user data changes will make some dedups fail. When that happens bees just abandons the extent immediately: a user data change will be handled in the next pass over the filesystem, but an unexpected kernel behavior needs bees code changes to correctly predict the new kernel behavior before the dedup can be reattempted. > --D > > > --Mark > > -- > > 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 > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 17, 2018 at 01:15:51AM -0400, Zygo Blaxell wrote: > On Sun, May 13, 2018 at 11:26:39AM -0700, Darrick J. Wong wrote: > > On Sun, May 13, 2018 at 06:21:52PM +0000, Mark Fasheh wrote: > > > On Fri, May 11, 2018 at 05:06:34PM -0700, Darrick J. Wong wrote: > > > > On Fri, May 11, 2018 at 12:26:51PM -0700, Mark Fasheh wrote: > > > > > Right now we return EINVAL if a process does not have permission to dedupe a > > > > > file. This was an oversight on my part. EPERM gives a true description of > > > > > the nature of our error, and EINVAL is already used for the case that the > > > > > filesystem does not support dedupe. > > > > > > > > > > Signed-off-by: Mark Fasheh <mfasheh@suse.de> > > > > > --- > > > > > fs/read_write.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/fs/read_write.c b/fs/read_write.c > > > > > index 77986a2e2a3b..8edef43a182c 100644 > > > > > --- a/fs/read_write.c > > > > > +++ b/fs/read_write.c > > > > > @@ -2038,7 +2038,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same) > > > > > info->status = -EINVAL; > > > > > } else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE) || > > > > > uid_eq(current_fsuid(), dst->i_uid))) { > > > > > - info->status = -EINVAL; > > > > > + info->status = -EPERM; > > > > > > > > Hmm, are we allowed to change this aspect of the kabi after the fact? > > > > > > > > Granted, we're only trading one error code for another, but will the > > > > existing users of this care? xfs_io won't and I assume duperemove won't > > > > either, but what about bees? :) > > > > > > Yeah if you see my initial e-mail I check bees and also rust-btrfs. I think > > > this is fine as we're simply expanding on an error code return. There's no > > > magic behavior expected with respect to these error codes either. > > > > Ok. No objections from me, then. > > > > Acked-by: Darrick J. Wong <darrick.wong@oracle.com> > > For what it's worth, no objection from me either. ;) > > bees runs only with admin privilege and will never hit the modified line. Awesome, thanks for the review Zygo. --Mark
diff --git a/fs/read_write.c b/fs/read_write.c index 77986a2e2a3b..8edef43a182c 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -2038,7 +2038,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same) info->status = -EINVAL; } else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE) || uid_eq(current_fsuid(), dst->i_uid))) { - info->status = -EINVAL; + info->status = -EPERM; } else if (file->f_path.mnt != dst_file->f_path.mnt) { info->status = -EXDEV; } else if (S_ISDIR(dst->i_mode)) {
Right now we return EINVAL if a process does not have permission to dedupe a file. This was an oversight on my part. EPERM gives a true description of the nature of our error, and EINVAL is already used for the case that the filesystem does not support dedupe. Signed-off-by: Mark Fasheh <mfasheh@suse.de> --- fs/read_write.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)