linux-kernel - Re: [PATCHv3 8/8] zram: correct typos

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y04LB3/hLrG00Zln@google.com>
Date:   Tue, 18 Oct 2022 11:10:15 +0900
From:   Sergey Senozhatsky <senozhatsky@...omium.org>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        Minchan Kim <minchan@...nel.org>,
        Nitin Gupta <ngupta@...are.org>, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org
Subject: Re: [PATCHv3 8/8] zram: correct typos

On (22/10/17 17:08), Andrew Morton wrote:
> On Sun,  9 Oct 2022 18:07:20 +0900 Sergey Senozhatsky <senozhatsky@...omium.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;