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

[Brian Foster <bfoster@redhat.com>]

On Mon, Jan 27, 2025 at 09:39:01PM -0800, Christoph Hellwig wrote:
> On Wed, Jan 22, 2025 at 08:34:33AM -0500, Brian Foster wrote:
> > +	size_t bytes = iomap_length(iter);
> 
> > +		bytes = min_t(u64, SIZE_MAX, bytes);
> 
> bytes needs to be a u64 for the min logic to work on 32-bit systems.
> 

Err.. I think there's another bug here. I changed iomap_iter_advance()
to return s64 so it could return length or an error, but never changed
bytes over from size_t.

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.

Another option could be to rework advance back to something like:

	int iomap_iter_advance(..., u64 *count);

... but where it returns 0 or -EIO and advances/updates *count directly.
That would mean I'd have to tweak some of the loop factoring and lift
out the error passthru assignment logic from iomap_iter(). The latter
doesn't seem like a big deal. It's mostly pointless after these changes.
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..

Brian