Received: by 2002:a25:31c3:0:0:0:0:0 with SMTP id x186csp346857ybx; Tue, 29 Oct 2019 19:59:15 -0700 (PDT) X-Google-Smtp-Source: APXvYqzpWAbQTNa2Tve/+cO1NpjKPSR8BZbxMYlw7/4mpxMTuAY/wXwG7eEqpevkMh0Fa4gR5fWY X-Received: by 2002:a17:906:770c:: with SMTP id q12mr6655155ejm.75.1572404355420; Tue, 29 Oct 2019 19:59:15 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1572404355; cv=none; d=google.com; s=arc-20160816; b=DOO0o0Sb0RQqa/5RLIV64NBh41syI7wWvisonL0K18YEpFeCTCs8YYdD2WjqCr92wE WR7DWuUSwIcugHdgYywazn960ZWnQ5ROheTN+jAD9S2Z7RobenMZwNVmOw+jZ8rPCyfH kWyhmfNpy/WMtFCCv3EJc0TKkqtA4kEfFxDe1baMjpPMkAwa1nDDRWd4AAF0tU0NxX6N zsGEyZPxTlKeBv1FZWScaYJtKrnxHEoU2JETMAcpJcegbiAq+D+CXbr0otRh1NzjyOX7 5t31juhCQ3Jldznz5G5iHncv813wgDd6IT15cY66gF+zT7nhJNNxIVVGMzom+sL6ur2V MUCg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:mail-followup-to :message-id:subject:cc:to:from:date:dkim-signature; bh=tE7wL1uKLwSnH0rdXLK9pPnuO6/icDxk60g0ygUJBu4=; b=TBp1K+TM9TjiYmd+z1ZDXcCYUiXHY3u8FLdhdD4CTWJMk3bMbbUAXqAxilw6Iv1JT2 fwk/2fUppSZs4KZNCP2xQ1MtwDZMfUyj+/WclFSm65LJis2VdilnSZijUMfbOv6KQyUO D4EDb9qXPbr+w9NQsSiprxg+iA9txJAADYKwS25stzWetvDLkmEKHbXk9piNbVaFJX/i BxbshLkoHEzbQ0dAUGop5ANFc1eDXLzNc1PY0942hVXhZYcN0UKhpohidiw63lnnglc+ 9vEWY3uzwO9vaS5wr6bGMjRYOKXE7slJBCBhJSNV7Lsub39+u845UaaxfFUy+LkgBunq Eb8g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=Z9FrYd1l; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id p21si488928edc.305.2019.10.29.19.58.51; Tue, 29 Oct 2019 19:59:15 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=Z9FrYd1l; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727193AbfJ3CzQ (ORCPT + 99 others); Tue, 29 Oct 2019 22:55:16 -0400 Received: from mail.kernel.org ([198.145.29.99]:38644 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726831AbfJ3CzP (ORCPT ); Tue, 29 Oct 2019 22:55:15 -0400 Received: from sol.localdomain (c-24-5-143-220.hsd1.ca.comcast.net [24.5.143.220]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 70A0120856; Wed, 30 Oct 2019 02:55:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1572404114; bh=PauVWSxjmuThe1M0wDRef8hgagGVTm4RCXliKrxBA+g=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Z9FrYd1l8EhJDfZXDF661fQwvMXBCgKXBd913ct2ii9dMZbVRVdOJc9qy3bwAc9Cd JvClmw0vq8n8IQMbmh4BhO0mi4pKy8rOidLS2N6XqyIh03D9hHlfx6Bo0K2Nz5fAgM 2RYWU5rkaUUg5tXvmkdm01Vuu4YiF4tlTjzNjlP0= Date: Tue, 29 Oct 2019 19:55:12 -0700 From: Eric Biggers To: Chao Yu Cc: Jaegeuk Kim , linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net Subject: Re: [PATCH 2/2] f2fs: support data compression Message-ID: <20191030025512.GA4791@sol.localdomain> Mail-Followup-To: Chao Yu , Jaegeuk Kim , linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net References: <20191022171602.93637-1-jaegeuk@kernel.org> <20191022171602.93637-2-jaegeuk@kernel.org> <20191027225006.GA321938@sol.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.12.2 (2019-09-21) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Oct 29, 2019 at 04:33:36PM +0800, Chao Yu wrote: > On 2019/10/28 6:50, Eric Biggers wrote: > >> +bool f2fs_is_compressed_page(struct page *page) > >> +{ > >> + if (!page_private(page)) > >> + return false; > >> + if (IS_ATOMIC_WRITTEN_PAGE(page) || IS_DUMMY_WRITTEN_PAGE(page)) > >> + return false; > >> + return *((u32 *)page_private(page)) == F2FS_COMPRESSED_PAGE_MAGIC; > >> +} > > > > This code implies that there can be multiple page private structures each of > > which has a different magic number. But I only see F2FS_COMPRESSED_PAGE_MAGIC. > > Where in the code is the other one(s)? > > I'm not sure I understood you correctly, did you mean it needs to introduce > f2fs_is_atomic_written_page() and f2fs_is_dummy_written_page() like > f2fs_is_compressed_page()? > No, I'm asking what is the case where the line *((u32 *)page_private(page)) == F2FS_COMPRESSED_PAGE_MAGIC returns false? > > > >> + > >> +static void f2fs_set_compressed_page(struct page *page, > >> + struct inode *inode, pgoff_t index, void *data, refcount_t *r) > >> +{ > >> + SetPagePrivate(page); > >> + set_page_private(page, (unsigned long)data); > >> + > >> + /* i_crypto_info and iv index */ > >> + page->index = index; > >> + page->mapping = inode->i_mapping; > >> + if (r) > >> + refcount_inc(r); > >> +} > > > > It isn't really appropriate to create fake pagecache pages like this. Did you > > consider changing f2fs to use fscrypt_decrypt_block_inplace() instead? > > We need to store i_crypto_info and iv index somewhere, in order to pass them to > fscrypt_decrypt_block_inplace(), where did you suggest to store them? > The same place where the pages are stored. > >> + > >> +void f2fs_destroy_compress_ctx(struct compress_ctx *cc) > >> +{ > >> + kvfree(cc->rpages); > >> +} > > > > The memory is allocated with kzalloc(), so why is it freed with kvfree() and not > > just kfree()? > > It was allocated by f2fs_*alloc() which will fallback to kvmalloc() once > kmalloc() failed. This seems to be a bug in f2fs_kmalloc() -- it inappropriately falls back to kvmalloc(). As per its name, it should only use kmalloc(). f2fs_kvmalloc() already exists, so it can be used when the fallback is wanted. > > >> +static int lzo_compress_pages(struct compress_ctx *cc) > >> +{ > >> + int ret; > >> + > >> + ret = lzo1x_1_compress(cc->rbuf, cc->rlen, cc->cbuf->cdata, > >> + &cc->clen, cc->private); > >> + if (ret != LZO_E_OK) { > >> + printk_ratelimited("%sF2FS-fs: lzo compress failed, ret:%d\n", > >> + KERN_ERR, ret); > >> + return -EIO; > >> + } > >> + return 0; > >> +} > > > > Why not using f2fs_err()? Same in lots of other places. > > We use printk_ratelimited at some points where we can afford to lose logs, > otherwise we use f2fs_{err,warn...} to record info as much as possible for > troubleshoot. > It used to be the case that f2fs_msg() was ratelimited. What stops it from spamming the logs now? The problem with a bare printk is that it doesn't show which filesystem instance the message is coming from. > >> + > >> + ret = cops->compress_pages(cc); > >> + if (ret) > >> + goto out_vunmap_cbuf; > >> + > >> + max_len = PAGE_SIZE * (cc->cluster_size - 1) - COMPRESS_HEADER_SIZE; > >> + > >> + if (cc->clen > max_len) { > >> + ret = -EAGAIN; > >> + goto out_vunmap_cbuf; > >> + } > > > > Since we already know the max length we're willing to compress to (the max > > length for any space to be saved), why is more space than that being allocated? > > LZ4_compress_default() will return an error if there isn't enough space, so that > > error could just be used as the indication to store the data uncompressed. > > AFAIK, there is no such common error code returned from all compression > algorithms indicating there is no room for limited target size, however we need > that information to fallback to write raw pages. Any better idea? > "Not enough room" is the only reasonable way for compression to fail, so all that's needed is the ability for compression to report errors at all. What specifically prevents this approach from working? > >> static void bio_post_read_processing(struct bio_post_read_ctx *ctx) > >> { > >> - /* > >> - * We use different work queues for decryption and for verity because > >> - * verity may require reading metadata pages that need decryption, and > >> - * we shouldn't recurse to the same workqueue. > >> - */ > > > > Why is it okay (i.e., no deadlocks) to no longer use different work queues for > > decryption and for verity? See the comment above which is being deleted. > > Could you explain more about how deadlock happen? or share me a link address if > you have described that case somewhere? > The verity work can read pages from the file which require decryption. I'm concerned that it could deadlock if the work is scheduled on the same workqueue. Granted, I'm not an expert in Linux workqueues, so if you've investigated this and determined that it's safe, can you explain why? - Eric