All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
To: Minchan Kim <minchan@kernel.org>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org,
	Joonsoo Kim <iamjoonsoo.kim@lge.com>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	kernel-team <kernel-team@lge.com>
Subject: Re: [PATCH 2/2] zram: do not count duplicated pages as compressed
Date: Tue, 16 May 2017 14:45:33 +0900	[thread overview]
Message-ID: <20170516054533.GA767@jagdpanzerIV.localdomain> (raw)
In-Reply-To: <20170516052605.GA5429@bbox>

On (05/16/17 14:26), Minchan Kim wrote:
[..]
> > +       /*
> > +        * Free memory associated with this sector
> > +        * before overwriting unused sectors.
> > +        */
> > +       zram_slot_lock(zram, index);
> > +       zram_free_page(zram, index);
> 
> Hmm, zram_free should happen only if the write is done successfully.
> Otherwise, we lose the valid data although write IO was fail.

but would this be correct? the data is not valid - we failed to store
the valid one. but instead we assure application that read()/swapin/etc.,
depending on the usage scenario, is successful (even though the data is
not what application really expects to see), application tries to use the
data from that page and probably crashes (dunno, for example page contained
hash tables with pointers that are not valid anymore, etc. etc.).

I'm not optimistic about stale data reads; it basically will look like
data corruption to the application.

> > +
> >         if (zram_same_page_write(zram, index, page))
> > -               return 0;
> > +               goto out_unlock;
> >  
> >         entry = zram_dedup_find(zram, page, &checksum);
> >         if (entry) {
> >                 comp_len = entry->len;
> > +               zram_set_flag(zram, index, ZRAM_DUP);
>
> In case of hitting dedup, we shouldn't increase compr_data_size.

no, we should not. you are right. my "... patch" is incomplete and
wrong. please don't pay too much attention to it.


> If we fix above two problems, do you think it's still cleaner?
> (I don't mean to be reluctant with your suggestion. Just a
>  real question to know your thought.:)

do you mean code duplication and stale data read?

I'd probably prefer to address stale data reads separately.
but it seems that stale reads fix will re-do parts of your
0002 patch and, at least potentially, reduce code duplication.

so we can go with your 0002 and then stale reads fix will try
to reduce code duplication (unless we want to have 4 places doing
the same thing :) )

	-ss

  reply	other threads:[~2017-05-16  5:45 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-15  7:41 [PATCH 1/2] zram: count same page write as page_stored Minchan Kim
2017-05-15  7:41 ` [PATCH 2/2] zram: do not count duplicated pages as compressed Minchan Kim
2017-05-16  1:30   ` Sergey Senozhatsky
2017-05-16  1:59     ` Minchan Kim
2017-05-16  2:36       ` Sergey Senozhatsky
2017-05-16  5:26         ` Minchan Kim
2017-05-16  5:45           ` Sergey Senozhatsky [this message]
2017-05-16  7:16             ` Minchan Kim
2017-05-16  7:36               ` Sergey Senozhatsky
2017-05-17  8:32                 ` Minchan Kim
2017-05-17  9:14                   ` Sergey Senozhatsky
2017-05-18  4:53                     ` Minchan Kim
2017-05-21  7:04                   ` Christoph Hellwig
2017-05-21  7:15                     ` Minchan Kim
2017-05-16  1:11 ` [PATCH 1/2] zram: count same page write as page_stored Sergey Senozhatsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170516054533.GA767@jagdpanzerIV.localdomain \
    --to=sergey.senozhatsky.work@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=iamjoonsoo.kim@lge.com \
    --cc=kernel-team@lge.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=minchan@kernel.org \
    --cc=sergey.senozhatsky@gmail.com \
    --subject='Re: [PATCH 2/2] zram: do not count duplicated pages as compressed' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.