Message-ID: <20200417153533.GB178621@google.com>
Date:   Fri, 17 Apr 2020 08:35:33 -0700
From:   Minchan Kim <minchan@...nel.org>
To:     Justin Gottula <justin@...ttula.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Nitin Gupta <ngupta@...are.org>, Jens Axboe <axboe@...nel.dk>,
        linux-block@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] zram: fix writeback_store returning zero in most
 situations

Hi Justin,

On Thu, Apr 16, 2020 at 05:45:31PM -0700, Justin Gottula wrote:
> On Thu, Apr 16, 2020 at 2:47 PM Minchan Kim <minchan@...nel.org> 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.

I agree it should be best effort.

> 
> 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).

Indeed.

What I wanted by my suggestion was that user could find the error
as soon as something popped up so they could judge what they could
as well as consistency of code structuring. But based on your comment,
they couldn't since several errors are overloaded here. In that case,
it makes my trial void. Another thought is if we could change writeback
work asynchronously(and sync right before returning to the user), it also
will make my trial void. So, yes, your suggestion is much sane.

Could you resend it with dealing with zram_bvec_read failure?
And change footer with following
Fixes: 3b82a051c101 ("zram: fix error return codes not being returned in writeback_store")
Cc: stable@...r.kernel.org

Powered by blists - more mailing lists