| Message ID | 1675341227-14-1-git-send-email-ruansy.fnst@fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fsdax: dax_unshare_iter() should return a valid length | expand |
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?
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;
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.
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);
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(+)