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.
