Re: [PATCH] zram: fix writeback_store returning zero in most situations

[Justin Gottula]

On Thu, Apr 16, 2020 at 2:47 PM Minchan Kim <minchan@xxxxxxxxxx> wrote:
> I couldn't remember why I wanted to do continue even though we knew
> the write was failure from the beginning.
>
> Couldn't we just bail out whenever we encounter the error?
> Sergey, Justin, what do you think?
>
> IMO, it would be more consistent with other error handling.

As far as being consistent with the rest of the error handling in the
loop, I think there's a reasonable distinction that can be drawn between
some of the error cases and others. With some, breaking out immediately is
obviously the correct action; but with others, it's not as clear-cut.

For the zram->wb_limit_enable && !zram->bd_wb_limit case, breaking out of
the loop and immediately returning makes complete sense: once we've hit
the writeback limit, there's nothing that could happen in future loop
iterations that could cause us to somehow _not_ be at the limit anymore.
So there's no reason to continue the loop.

Similarly, for the alloc_block_bdev(zram) failure case, breaking out of
the loop and immediately returning also makes complete sense: if we've
already run out of blocks on the backing device, then continuing to loop
isn't going to result in is somehow ending up with more backing device
blocks available. So there's no reason to continue the loop in that case
either.

With the zram_bvec_read and submit_bio_wait failure cases, though, it's
not nearly as clear that a failure on the current slot _automatically_
guarantees that all future slots would also have errors. For example, in
the zram_bvec_read failure case, it's entirely possible that the
decompression code in the currently-used compression backend might have
had a problem with the current buffer; but that doesn't necessarily seem
like a clear-cut indication that the same decompression error would
definitely happen on later iterations of the loop: in fact, many of the
later slots could very well be ZRAM_HUGE or ZRAM_SAME, and so they would
avoid that type of error entirely. And in the submit_bio_wait failure
case, it's also conceivable that the backing device may have had some sort
of write error with the current block and data buffer, but that the same
error might not happen again on future iterations of the loop when writing
a different buffer to potentially an entirely different block. (Especially
if the backing device happens to be using some kind of complicated driver,
like one of the weirder LVM/device-mapper backends or a network block
device.)

So, at least when it comes to "are these particular failure cases the kind
where it would certainly make no sense to continue with future loop
iterations because we would definitely have the same failures then too",
there's a reasonable argument that the zram_bvec_read and submit_bio_wait
error cases don't necessarily fit that logic and so potentially shouldn't
immediately break and return.

A couple of other considerations came to mind while thinking about this:

1. From the standpoint of the user, what should they reasonably be
expecting to happen when writing "huge"/"idle" to the writeback file? A
best-effort attempt at doing all possible block writebacks (of the
requested type) that can possibly be successfully accomplished? Or,
writeback of only as many blocks as can be done until a (possibly
transient or not-applicable-to-all-blocks) error pops up, and then
stopping at that point? I tend to think that the former makes a bit more
sense, but I don't know for sure if it's the definite Right Answer.

2. There is a bit of a predicament with the keep-going-even-if-an-error-
happened approach. There could be multiple submit_bio_wait failures (e.g.
due to a few blocks that couldn't be written-back due to transient write
errors on the backing device), each with their own error return value; but
we can only return one error code from writeback_store. So how do we even
communicate a multiple-errors-happened situation to the user properly? My
patch, as originally written, would give the user an errno corresponding
to the last submit_bio_wait failure to happen; but of course that leaves
out any other error codes that may have also come up on prior write
attempts that failed (and those failures could have been for different
reasons). And then also, it's entirely possible that submit_bio_wait may
return -EIO or -ENOSPC or -ENOMEM, which would be ambiguous with the other
meanings of those error codes from the writeback_store function itself
(-EIO for WB limit, -ENOSPC for no free blocks on backing device, -ENOMEM
if couldn't allocate the temporary page).

As for the main issue of whether or not to break out of the loop in the
event of a backing device write error, I think it probably makes more
sense not to break out. But it could probably be argued either way.