* [PATCH] fsdax: dax_unshare_iter() should return a valid length
@ 2023-02-02 12:33 Shiyang Ruan
2023-02-02 23:01 ` Matthew Wilcox
0 siblings, 1 reply; 4+ messages in thread
From: Shiyang Ruan @ 2023-02-02 12:33 UTC (permalink / raw)
To: linux-xfs, nvdimm, linux-fsdevel
Cc: djwong, david, dan.j.williams, akpm, ruansy.fnst
The copy_mc_to_kernel() will return 0 if it executed successfully.
Then the return value should be set to the length it copied.
Fixes: d984648e428b ("fsdax,xfs: port unshare to fsdax")
Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
---
fs/dax.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/dax.c b/fs/dax.c
index c48a3a93ab29..a5b4deb5def3 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -1274,6 +1274,7 @@ static s64 dax_unshare_iter(struct iomap_iter *iter)
ret = copy_mc_to_kernel(daddr, saddr, length);
if (ret)
ret = -EIO;
+ ret = length;
out_unlock:
dax_read_unlock(id);
--
2.39.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] fsdax: dax_unshare_iter() should return a valid length
2023-02-02 12:33 [PATCH] fsdax: dax_unshare_iter() should return a valid length Shiyang Ruan
@ 2023-02-02 23:01 ` Matthew Wilcox
2023-02-03 0:23 ` Andrew Morton
0 siblings, 1 reply; 4+ messages in thread
From: Matthew Wilcox @ 2023-02-02 23:01 UTC (permalink / raw)
To: Shiyang Ruan
Cc: linux-xfs, nvdimm, linux-fsdevel, djwong, david, dan.j.williams,
akpm
On Thu, Feb 02, 2023 at 12:33:47PM +0000, Shiyang Ruan wrote:
> The copy_mc_to_kernel() will return 0 if it executed successfully.
> Then the return value should be set to the length it copied.
>
> Fixes: d984648e428b ("fsdax,xfs: port unshare to fsdax")
> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
> ---
> fs/dax.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/dax.c b/fs/dax.c
> index c48a3a93ab29..a5b4deb5def3 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -1274,6 +1274,7 @@ static s64 dax_unshare_iter(struct iomap_iter *iter)
> ret = copy_mc_to_kernel(daddr, saddr, length);
> if (ret)
> ret = -EIO;
> + ret = length;
Umm. Surely should be:
else
ret = length;
otherwise you've just overwritten the -EIO.
And maybe this should be:
ret = length - copy_mc_to_kernel(daddr, saddr, length);
if (ret < length)
ret = -EIO;
What do you think?
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] fsdax: dax_unshare_iter() should return a valid length
2023-02-02 23:01 ` Matthew Wilcox
@ 2023-02-03 0:23 ` Andrew Morton
2023-02-03 0:42 ` Dan Williams
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Morton @ 2023-02-03 0:23 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Shiyang Ruan, linux-xfs, nvdimm, linux-fsdevel, djwong, david,
dan.j.williams
On Thu, 2 Feb 2023 23:01:23 +0000 Matthew Wilcox <willy@infradead.org> wrote:
> On Thu, Feb 02, 2023 at 12:33:47PM +0000, Shiyang Ruan wrote:
> > The copy_mc_to_kernel() will return 0 if it executed successfully.
> > Then the return value should be set to the length it copied.
> >
> > Fixes: d984648e428b ("fsdax,xfs: port unshare to fsdax")
> > Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
> > ---
> > fs/dax.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/fs/dax.c b/fs/dax.c
> > index c48a3a93ab29..a5b4deb5def3 100644
> > --- a/fs/dax.c
> > +++ b/fs/dax.c
> > @@ -1274,6 +1274,7 @@ static s64 dax_unshare_iter(struct iomap_iter *iter)
> > ret = copy_mc_to_kernel(daddr, saddr, length);
> > if (ret)
> > ret = -EIO;
> > + ret = length;
>
> Umm. Surely should be:
>
> else
> ret = length;
>
> otherwise you've just overwritten the -EIO.
yup
> And maybe this should be:
>
> ret = length - copy_mc_to_kernel(daddr, saddr, length);
> if (ret < length)
> ret = -EIO;
>
not a fan of giving `ret' a temporary new meaning like that. If it was
copied = length - copy_mc_to_kernel(daddr, saddr, length);
if (copied < length)
ret = -EIO;
then it would be clear.
Clearer, methinks:
if (copy_mc_to_kernel(daddr, saddr, length) == 0)
ret = length;
else
ret = -EIO;
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] fsdax: dax_unshare_iter() should return a valid length
2023-02-03 0:23 ` Andrew Morton
@ 2023-02-03 0:42 ` Dan Williams
0 siblings, 0 replies; 4+ messages in thread
From: Dan Williams @ 2023-02-03 0:42 UTC (permalink / raw)
To: Andrew Morton, Matthew Wilcox
Cc: Shiyang Ruan, linux-xfs, nvdimm, linux-fsdevel, djwong, david,
dan.j.williams
Andrew Morton wrote:
> On Thu, 2 Feb 2023 23:01:23 +0000 Matthew Wilcox <willy@infradead.org> wrote:
>
> > On Thu, Feb 02, 2023 at 12:33:47PM +0000, Shiyang Ruan wrote:
> > > The copy_mc_to_kernel() will return 0 if it executed successfully.
> > > Then the return value should be set to the length it copied.
> > >
> > > Fixes: d984648e428b ("fsdax,xfs: port unshare to fsdax")
> > > Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
[..]
>
> Clearer, methinks:
>
> if (copy_mc_to_kernel(daddr, saddr, length) == 0)
> ret = length;
> else
> ret = -EIO;
>
That looks good to me.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-02-03 0:43 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-02 12:33 [PATCH] fsdax: dax_unshare_iter() should return a valid length Shiyang Ruan
2023-02-02 23:01 ` Matthew Wilcox
2023-02-03 0:23 ` Andrew Morton
2023-02-03 0:42 ` Dan Williams
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).