From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-f193.google.com ([209.85.210.193]:36910 "EHLO mail-pf1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726381AbeHCGpz (ORCPT ); Fri, 3 Aug 2018 02:45:55 -0400 Date: Fri, 3 Aug 2018 13:51:21 +0900 From: Minchan Kim To: Sergey Senozhatsky Cc: Andrew Morton , LKML , Tino Lehnig , stable@vger.kernel.org, Jens Axboe Subject: Re: [PATCH 1/2] zram: remove BD_CAP_SYNCHRONOUS_IO with writeback feature Message-ID: <20180803045121.GC86818@rodete-desktop-imager.corp.google.com> References: <20180802051112.86174-1-minchan@kernel.org> <20180802141304.d0589ddc5f8213429ab3b565@linux-foundation.org> <20180803023929.GA7500@jagdpanzerIV> <20180803030019.GB86818@rodete-desktop-imager.corp.google.com> <20180803041302.GB502@jagdpanzerIV> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180803041302.GB502@jagdpanzerIV> Sender: stable-owner@vger.kernel.org List-ID: On Fri, Aug 03, 2018 at 01:13:02PM +0900, Sergey Senozhatsky wrote: > Hi Minchan, > > On (08/03/18 12:00), Minchan Kim wrote: > > > "Device is so fast that asynchronous IO would be inefficient." > > > > > > Which is not the reason why BDI_CAP_SYNCHRONOUS_IO is used by ZRAM. > > > Probably, the comment needs to be updated as well. > > > > I couldn't catch your point. Could you clarify a little bit more? > > What do you want to correct for the comment? > > > > > Both SWP_SYNCHRONOUS_IO and BDI_CAP_SYNCHRONOUS_IO tend to pivot > > > "efficiency" [looking at the comments], but in ZRAM's case the whole > > > reason to use SYNC IO is a race condition and user-after-free that > > > follows. > > > > Actually, it's not whole reason. As I wrote down, without it, swap_readpage > > waits the IO completion for a long time by blk_poll so it causes system > > sluggish problem when device is slow(e.g., zram with backing device). > > Sure, this is problem #1. But slow swap device probably doesn't do any > irreversible harm to the system. Unlike use-after-free, which does. Thus > use-after-free is a problem #0. BDI_CAP_SYNCHRONOUS_IO comment doesn't > mention problem #0; it talks about problem #1 only. So, nothing serious, > just wanted to point that out. > > So we probably can make ZRAM always ASYNC when WB is enabled. > > > Or... maybe we can make swap out to be SYNC and perform WB in background. > In __zram_bvec_write() we can always write compressed object to zmalloc, > even the huge ones. > Things to note: > a) even when WB is enabled we still allocate huge classes > b) even when WB is enabled we still may use those huge classes (consider > a case when backing devices is full) > > So huge classes are still there and we still use them. So let's use > them? > > For a huge object, after we stored it into zsmalloc, we can schedule a WB > work, which would: > a) write that particular object (page) to the backing device > b) mark entry as WB entry > c) remove object from zsmalloc, unlock necessary locks > > So swap in should either see object in zsmalloc or on backing device. > How does this sound? > > And reading from a backing device can always be SYNC. Can it? > Am I missing something? AFAIK, onging writeback page couldn't freed so it was not writeabck problem. What I'm tryig to fix is read part. If we use swapcache, it shouldn't be a problem either because swapcache has a reference count and we should wait PG_lock release before the freeing from the swapcache so there is no race condition. However, by the skip swapcache logic, we don't have a refcount any longer. I think we can hold a new refcount in zram driver itself. With that, we could get both benefits from writeback feature and skip swapcache. However, I decided, at this moment, going this simple way for stable-material to solve #0 and #1 problems at the same time. Thanks. > > -ss