Re: [PATCH v2 6/7] iomap: advance the iter directly on unshare range

[Brian Foster <bfoster@redhat.com>]

On Tue, Jan 28, 2025 at 09:56:10PM -0800, Christoph Hellwig wrote:
> On Tue, Jan 28, 2025 at 12:59:09PM -0500, Brian Foster wrote:
> > But that raises another question. I'd want bytes to be s64 here to
> > support the current factoring, but iomap_length() returns a u64. In
> > poking around a bit I _think_ this is practically safe because the high
> > level operations are bound by loff_t (int64_t), so IIUC that means we
> > shouldn't actually see a length that doesn't fit in s64.
> > 
> > That said, that still seems a bit grotty. Perhaps one option could be to
> > tweak iomap_length() to return something like this:
> > 
> > 	min_t(u64, SSIZE_MAX, end);
> > 
> > ... to at least makes things explicit.
> 
> Yeah.  I'm actually not sure why went want to support 64-bit ranges.
> I don't even remember if this comes from Dave's really first version
> or was my idea, but in hindsight just sticking to ssize_t bounds
> would have been smarter.
> 

Ok, thanks.

> > I'd guess the (i.e. iomap_file_unshare()) loop logic would look more
> > like:
> > 
> > 	do {
> > 		...
> > 		ret = iomap_iter_advance(iter, &bytes);
> > 	} while (!ret && bytes > 0);
> > 
> > 	return ret;
> > 
> > Hmm.. now that I write it out that doesn't seem so bad. It does clean up
> > the return path a bit. I think I'll play around with that, but let me
> > know if there are other thoughts or ideas..
> 
> Given that all the kernel read/write code mixes up bytes and negative
> return values I think doing that in iomap is also fine.  But you are
> deeper into the code right now, and if you think splitting the errno
> and bytes is cleaner that sounds perfectly fine to me as well.  In
> general not overloading a single return value with two things tends
> to lead to cleaner code.
> 

Eh, I like the factoring that the combined return allows better, but I
don't want to get too clever and introduce type issues and whatnot in
the middle of these patches if I can help it. From what I see so far the
change to split out the error return uglifies things slightly in
iomap_iter(), but the flipside is that with the error check lifted out
the advance call from iomap_iter() can go away completely once
everything is switched over.

So if we do go with the int return for now (still testing), I might
revisit a change back to a combined s64 return (perhaps along with the
iomap_length() tweak above) in the future as a standalone cleanup when
this is all more settled and I have more mental bandwidth to think about
it. Thanks for the input.

> Although the above sniplet (I´m not sure how representative it is
> anyway) would be a bit nicer as the slightly more verbose version
> below:
> 
> 	do {
> 		...
> 		ret = iomap_iter_advance(iter, &bytes);
> 		if (ret)
> 			return ret;
> 	} while (bytes > 0);
> 

Ack.

Brian