Received: by 2002:a25:4158:0:0:0:0:0 with SMTP id o85csp4266913yba; Mon, 29 Apr 2019 17:01:16 -0700 (PDT) X-Google-Smtp-Source: APXvYqxtVf3n4E6DieTtMrk007dcdnqgDxNyZgTGHXOUOOuQI3UUjmo6QsZXOQbu+7jAs/qo2xx9 X-Received: by 2002:a63:5202:: with SMTP id g2mr662930pgb.317.1556582476146; Mon, 29 Apr 2019 17:01:16 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1556582476; cv=none; d=google.com; s=arc-20160816; b=oOVHHvSsMHY9MFJl8EQu9d5CmePENA0tpeAseRt2G0Jkosrr2y9MlZU8bEUP5JS1sG RtCxT6PItE8xyUrRerK4tjSGcut3lj9gLd5pncwselZnq7cpoUUzCUcOD4ukj2HJOft0 X54ybT+ujTInT8S0fTgCbIoFLLP6UpkvaLI/aLPDNTNm75xNUyfUN4YekbtNe4hNNvri XZ9wzqrxj+zvOcqITa1TIwv3Bb7u934MSbeQL/oqqKYGJPdcIVEIVLctnWoGJM3lJEkr +Y9c5Y0LXdMGkBISJ/HX75oO/xwDHK1E3BzJIwQuN2LGnYs1ImFAY4Lj7JngZpqSwOs1 8anQ== 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:message-id:subject:cc :to:from:date:dkim-signature; bh=LyJ2qfIFhyUtaiZ2e82Fk4uVtcPIP/CLwVRMa42iuf0=; b=zTy90hXZg0RRqYneS9KWOMySG04+2b0gvfsDOFEcX0HqU+V3fgZvufv0NwRXesMNL6 /crVGeootyC1jUeYIFQg39wxY//Z4Fc/WUtH/nagDOvFgyxKtYGOag7S6QtsuYz4+gfd kaKbFwAyzxZoJWABsokUUFDqohh7R4n52qLANkdoxhHfSSCkiR2sJ31JdZH/D6vzs4rT aWqdXrfZW5A/j7pYm336u1NfTTxv0GceEr1x6pWr4vk6OQkxdFR961BUvE4mAh6hYmcR 7KlR+zF/XvUQzp6UR3frUFJq+ntZZWpVPesIHZKp83QZuPZOOTsTbkBTFcO8R9BGdyI8 BT2A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=v6TBNIAr; spf=pass (google.com: best guess record for domain of linux-ext4-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-ext4-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 s9si36602916pfa.282.2019.04.29.17.00.54; Mon, 29 Apr 2019 17:01:16 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-ext4-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=v6TBNIAr; spf=pass (google.com: best guess record for domain of linux-ext4-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-ext4-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 S1729220AbfD3AAc (ORCPT + 99 others); Mon, 29 Apr 2019 20:00:32 -0400 Received: from mail.kernel.org ([198.145.29.99]:52984 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729116AbfD3AAc (ORCPT ); Mon, 29 Apr 2019 20:00:32 -0400 Received: from gmail.com (unknown [104.132.1.77]) (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 372F421655; Tue, 30 Apr 2019 00:00:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1556582430; bh=6aSKDR1uKVKH7OuvG9FmFXPziiMX8kv+4PyT8PGgWtU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=v6TBNIArS5hw3/PahnC1A1h6StmNSpYjrmU+VMSsRvM61l+/2oSOeD3i+tIsYaaPw VXJZJPKFZPzm3iaxMIfnf8exD3VrclcC1Bu9re+gpqo2Ek28fxQx+iRvayf9TCg0bh zQ6Pgx4Q+qqcJRb6+k9lQhs85t/VqK4tWuWXBXOc= Date: Mon, 29 Apr 2019 17:00:28 -0700 From: Eric Biggers To: Chandan Rajendra Cc: linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, linux-fscrypt@vger.kernel.org, tytso@mit.edu, adilger.kernel@dilger.ca, jaegeuk@kernel.org, yuchao0@huawei.com, hch@infradead.org Subject: Re: [PATCH V2 02/13] Consolidate "read callbacks" into a new file Message-ID: <20190430000027.GB251866@gmail.com> References: <20190428043121.30925-1-chandan@linux.ibm.com> <20190428043121.30925-3-chandan@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190428043121.30925-3-chandan@linux.ibm.com> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org Hi Chandan, On Sun, Apr 28, 2019 at 10:01:10AM +0530, Chandan Rajendra wrote: > The "read callbacks" code is used by both Ext4 and F2FS. Hence to > remove duplicity, this commit moves the code into > include/linux/read_callbacks.h and fs/read_callbacks.c. > > The corresponding decrypt and verity "work" functions have been moved > inside fscrypt and fsverity sources. With these in place, the read > callbacks code now has to just invoke enqueue functions provided by > fscrypt and fsverity. > > Signed-off-by: Chandan Rajendra > --- > fs/Kconfig | 4 + > fs/Makefile | 4 + > fs/crypto/Kconfig | 1 + > fs/crypto/bio.c | 23 ++--- > fs/crypto/crypto.c | 17 +-- > fs/crypto/fscrypt_private.h | 3 + > fs/ext4/ext4.h | 2 - > fs/ext4/readpage.c | 183 +++++---------------------------- > fs/ext4/super.c | 9 +- > fs/f2fs/data.c | 148 ++++---------------------- > fs/f2fs/super.c | 9 +- > fs/read_callbacks.c | 136 ++++++++++++++++++++++++ > fs/verity/Kconfig | 1 + > fs/verity/verify.c | 12 +++ > include/linux/fscrypt.h | 20 +--- > include/linux/read_callbacks.h | 21 ++++ > 16 files changed, 251 insertions(+), 342 deletions(-) > create mode 100644 fs/read_callbacks.c > create mode 100644 include/linux/read_callbacks.h > For easier review, can you split this into multiple patches? Ideally the ext4 and f2fs patches would be separate, but if that's truly not possible due to interdependencies it seems you could at least do: 1. Introduce the read_callbacks. 2. Convert encryption to use the read_callbacks. 3. Remove union from struct fscrypt_context. Also: just FYI, fs-verity isn't upstream yet, and in the past few months I haven't had much time to work on it. So you might consider arranging your series so that initially just fscrypt is supported. That will be useful on its own, for block_size < PAGE_SIZE support. Then fsverity can be added later. > diff --git a/fs/Kconfig b/fs/Kconfig > index 97f9eb8df713..03084f2dbeaf 100644 > --- a/fs/Kconfig > +++ b/fs/Kconfig > @@ -308,6 +308,10 @@ config NFS_COMMON > depends on NFSD || NFS_FS || LOCKD > default y > > +config FS_READ_CALLBACKS > + bool > + default n 'default n' is unnecesary, since 'n' is already the default. > + > source "net/sunrpc/Kconfig" > source "fs/ceph/Kconfig" > source "fs/cifs/Kconfig" > diff --git a/fs/Makefile b/fs/Makefile > index 9dd2186e74b5..e0c0fce8cf40 100644 > --- a/fs/Makefile > +++ b/fs/Makefile > @@ -21,6 +21,10 @@ else > obj-y += no-block.o > endif > > +ifeq ($(CONFIG_FS_READ_CALLBACKS),y) > +obj-y += read_callbacks.o > +endif > + This can be simplified to: obj-$(CONFIG_FS_READ_CALLBACKS) += read_callbacks.o > diff --git a/fs/read_callbacks.c b/fs/read_callbacks.c > new file mode 100644 > index 000000000000..b6d5b95e67d7 > --- /dev/null > +++ b/fs/read_callbacks.c > @@ -0,0 +1,136 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * This file tracks the state machine that needs to be executed after reading > + * data from files that are encrypted and/or have verity metadata associated > + * with them. > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define NUM_PREALLOC_POST_READ_CTXS 128 > + > +static struct kmem_cache *read_callbacks_ctx_cache; > +static mempool_t *read_callbacks_ctx_pool; > + > +/* Read callback state machine steps */ > +enum read_callbacks_step { > + STEP_INITIAL = 0, > + STEP_DECRYPT, > + STEP_VERITY, > +}; > + > +void end_read_callbacks(struct bio *bio) > +{ > + struct page *page; > + struct bio_vec *bv; > + int i; > + struct bvec_iter_all iter_all; > + > + bio_for_each_segment_all(bv, bio, i, iter_all) { > + page = bv->bv_page; > + > + BUG_ON(bio->bi_status); > + > + if (!PageError(page)) > + SetPageUptodate(page); > + > + unlock_page(page); > + } > + if (bio->bi_private) > + put_read_callbacks_ctx(bio->bi_private); > + bio_put(bio); > +} > +EXPORT_SYMBOL(end_read_callbacks); end_read_callbacks() is only called by read_callbacks() just below, so it should be 'static'. > + > +struct read_callbacks_ctx *get_read_callbacks_ctx(struct inode *inode, > + struct bio *bio, > + pgoff_t index) > +{ > + unsigned int read_callbacks_steps = 0; Rename 'read_callbacks_steps' => 'enabled_steps', since it's clear from context. > + struct read_callbacks_ctx *ctx = NULL; > + > + if (IS_ENCRYPTED(inode) && S_ISREG(inode->i_mode)) > + read_callbacks_steps |= 1 << STEP_DECRYPT; > +#ifdef CONFIG_FS_VERITY > + if (inode->i_verity_info != NULL && > + (index < ((i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT))) > + read_callbacks_steps |= 1 << STEP_VERITY; > +#endif To avoid the #ifdef, this should probably be made a function in fsverity.h. > + if (read_callbacks_steps) { > + ctx = mempool_alloc(read_callbacks_ctx_pool, GFP_NOFS); > + if (!ctx) > + return ERR_PTR(-ENOMEM); > + ctx->bio = bio; > + ctx->inode = inode; > + ctx->enabled_steps = read_callbacks_steps; > + ctx->cur_step = STEP_INITIAL; > + bio->bi_private = ctx; > + } > + return ctx; > +} > +EXPORT_SYMBOL(get_read_callbacks_ctx); The callers don't actually use the returned read_callbacks_ctx. Instead, they rely on this function storing it in ->bi_private. So, this function should just return an error code, and it should be renamed. Perhaps: int read_callbacks_setup_bio(struct inode *inode, struct bio *bio, pgoff_t first_pgoff); Please rename 'index' to 'first_pgoff' to make it clearer what it is, given that a bio can contain many pages. Please add kerneldoc for this function. - Eric