On (22/10/17 17:08), Andrew Morton wrote:
> On Sun, 9 Oct 2022 18:07:20 +0900 Sergey Senozhatsky <senozhatsky@chromium.org> wrote:
>
> > Trivial comment typos fixes.
> >
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -759,7 +759,7 @@ static ssize_t writeback_store(struct device *dev,
> > zram_slot_unlock(zram, index);
> > /*
> > * Return last IO error unless every IO were
> > - * not suceeded.
> > + * not succeeded.
>
> That's a pretty awkward sentence. Why not "unless every IO failed".
>
> If that's indeed what we're doing here. Sounds odd. What do we return
> if all IOs indeed failed?
Hmm, yes, I didn't consider re-phrasing this comment but we probably
should do so. What we have there is
while (nr_pages_to_write--) {
err = submit_bio_wait();
if (err) {
ret = err;
continue;
}
}
return ret;
zram objects are independent and bio errors on individual writes are
non-fatal, if we failed to write-back a zram object (page) we just
continue and try to write the next one; at the same time we need to
signal user-space that some of those writes failed (doesn't matter
which ones or how many). That loop used to look as follows (as far
as I can tell):
while (nr_pages_to_write--) {
ret = submit_bio_wait();
}
return ret;
Notice how `ret' would get overwritten all the time, so we if we had,
say, a successful submit_bio_wait, then an unsuccessful one and a
successful one again, we would lose the track of the bio error that
happened on the second iteration and will always return 0 to user-space.
*Unless* the last (or all) submit_bio_wait() also failed, in which case
`ret' would hold the correct error code.
Will something like this look less awkward to you?
---
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index ecbc5963b5b8..23f1655b7837 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -758,8 +758,12 @@ static ssize_t writeback_store(struct device *dev,
zram_clear_flag(zram, index, ZRAM_IDLE);
zram_slot_unlock(zram, index);
/*
- * Return last IO error unless every IO were
- * not succeeded.
+ * BIO errors are not fatal, we continue and simply
+ * attempt to writeback the remaining objects (pages).
+ * At the same time we need to signal user-space that
+ * some writes (at least one, but also could be all of
+ * them) were not successful and we do so by returning
+ * the most recent BIO error.
*/
ret = err;
continue;