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

[Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>]

On Thu, 2 Feb 2023 23:01:23 +0000 Matthew Wilcox <willy@xxxxxxxxxxxxx> 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@xxxxxxxxxxx>
> > ---
> >  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;