Re: [PATCH] fsdax: dax_unshare_iter() should return a valid length

[Andrew Morton]

On Thu, 2 Feb 2023 23:01:23 +0000 Matthew Wilcox <wi...@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.f...@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;