| Message ID | 31be6e0896eba59c06eb9d3d137b214f7220cc53.1588894359.git.skhan@linuxfoundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: avoid fdput() after failed fdget() | expand |
On Thu, May 07, 2020 at 05:57:09PM -0600, Shuah Khan wrote: > Fix ksys_sync_file_range() to avoid fdput() after a failed fdget(). > fdput() doesn't do fput() on this file since FDPUT_FPUT isn't set > in fd.flags. Fix it anyway since failed fdget() doesn't require > a fdput(). > > This was introdcued in a commit to add sync_file_range() helper. Er... What's the point microoptimizing the slow path here?
On Fri, May 08, 2020 at 01:05:09AM +0100, Al Viro wrote: > On Thu, May 07, 2020 at 05:57:09PM -0600, Shuah Khan wrote: > > Fix ksys_sync_file_range() to avoid fdput() after a failed fdget(). > > fdput() doesn't do fput() on this file since FDPUT_FPUT isn't set > > in fd.flags. Fix it anyway since failed fdget() doesn't require > > a fdput(). > > > > This was introdcued in a commit to add sync_file_range() helper. > > Er... What's the point microoptimizing the slow path here? PS: I'm not saying the patch is incorrect, but Fixes: is IMO over the top. And looking at that thing, { struct fd f = fdget(fd); int ret; if (unlikely(!f.file)) return -EBADF; ret = sync_file_range(f.file, offset, nbytes, flags); fdput(f); return ret; } might be cleaner, but that's a matter of taste...
On Fri, May 08, 2020 at 01:24:22AM +0100, Al Viro wrote: > On Fri, May 08, 2020 at 01:05:09AM +0100, Al Viro wrote: > > On Thu, May 07, 2020 at 05:57:09PM -0600, Shuah Khan wrote: > > > Fix ksys_sync_file_range() to avoid fdput() after a failed fdget(). > > > fdput() doesn't do fput() on this file since FDPUT_FPUT isn't set > > > in fd.flags. Fix it anyway since failed fdget() doesn't require > > > a fdput(). > > > > > > This was introdcued in a commit to add sync_file_range() helper. > > > > Er... What's the point microoptimizing the slow path here? > > PS: I'm not saying the patch is incorrect, but Fixes: is IMO over the > top. And looking at that thing, > { > struct fd f = fdget(fd); > int ret; > > if (unlikely(!f.file)) > return -EBADF; > > ret = sync_file_range(f.file, offset, nbytes, flags); > fdput(f); > return ret; > } > > might be cleaner, but that's a matter of taste... This makes it easier to read. Luis
On 5/7/20 8:21 PM, Luis Chamberlain wrote: > On Fri, May 08, 2020 at 01:24:22AM +0100, Al Viro wrote: >> On Fri, May 08, 2020 at 01:05:09AM +0100, Al Viro wrote: >>> On Thu, May 07, 2020 at 05:57:09PM -0600, Shuah Khan wrote: >>>> Fix ksys_sync_file_range() to avoid fdput() after a failed fdget(). >>>> fdput() doesn't do fput() on this file since FDPUT_FPUT isn't set >>>> in fd.flags. Fix it anyway since failed fdget() doesn't require >>>> a fdput(). >>>> >>>> This was introdcued in a commit to add sync_file_range() helper. >>> >>> Er... What's the point microoptimizing the slow path here? >> >> PS: I'm not saying the patch is incorrect, but Fixes: is IMO over the >> top. And looking at that thing, >> { >> struct fd f = fdget(fd); >> int ret; >> >> if (unlikely(!f.file)) >> return -EBADF; >> >> ret = sync_file_range(f.file, offset, nbytes, flags); >> fdput(f); >> return ret; >> } >> >> might be cleaner, but that's a matter of taste... > > This makes it easier to read. > Yes it does. I will make the changes and send v2. thanks, -- Shuah
diff --git a/fs/sync.c b/fs/sync.c index 4d1ff010bc5a..faaff835ef12 100644 --- a/fs/sync.c +++ b/fs/sync.c @@ -369,10 +369,11 @@ int ksys_sync_file_range(int fd, loff_t offset, loff_t nbytes, ret = -EBADF; f = fdget(fd); - if (f.file) + if (f.file) { ret = sync_file_range(f.file, offset, nbytes, flags); + fdput(f); + } - fdput(f); return ret; }
Fix ksys_sync_file_range() to avoid fdput() after a failed fdget(). fdput() doesn't do fput() on this file since FDPUT_FPUT isn't set in fd.flags. Fix it anyway since failed fdget() doesn't require a fdput(). This was introdcued in a commit to add sync_file_range() helper. Fixes: 22f96b3808c1 ("fs: add sync_file_range() helper") Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> --- fs/sync.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)