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
