From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751571AbdEPHQr (ORCPT ); Tue, 16 May 2017 03:16:47 -0400 Received: from LGEAMRELO12.lge.com ([156.147.23.52]:47092 "EHLO lgeamrelo12.lge.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751138AbdEPHQq (ORCPT ); Tue, 16 May 2017 03:16:46 -0400 X-Original-SENDERIP: 156.147.1.126 X-Original-MAILFROM: minchan@kernel.org X-Original-SENDERIP: 10.177.220.163 X-Original-MAILFROM: minchan@kernel.org Date: Tue, 16 May 2017 16:16:44 +0900 From: Minchan Kim To: Sergey Senozhatsky Cc: Andrew Morton , linux-kernel@vger.kernel.org, Joonsoo Kim , Sergey Senozhatsky , kernel-team Subject: Re: [PATCH 2/2] zram: do not count duplicated pages as compressed Message-ID: <20170516071644.GA6224@bbox> References: <1494834068-27004-1-git-send-email-minchan@kernel.org> <1494834068-27004-2-git-send-email-minchan@kernel.org> <20170516013022.GB10262@jagdpanzerIV.localdomain> <20170516015919.GA5233@bbox> <20170516023615.GC10262@jagdpanzerIV.localdomain> <20170516052605.GA5429@bbox> <20170516054533.GA767@jagdpanzerIV.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170516054533.GA767@jagdpanzerIV.localdomain> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 16, 2017 at 02:45:33PM +0900, Sergey Senozhatsky wrote: > 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. Hmm, I don't understand what you say. My point is zram_free_page should be done only if whoe write operation is successful. With you change, following situation can happens. write block 4, 'all A' -> success read block 4, 'all A' verified -> Good write block 4, 'all B' -> but failed with ENOMEM read block 4 expected 'all A' but 'all 0' -> Oops It is the problem what I pointed out. If I miss something, could you elaborate it a bit? Thanks! > > > > + > > > 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