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
