From patchwork Wed Feb 21 18:53:31 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Biggers X-Patchwork-Id: 10233817 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id F1874602A7 for ; Wed, 21 Feb 2018 18:53:38 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id DB192283D1 for ; Wed, 21 Feb 2018 18:53:38 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id CFA61283D8; Wed, 21 Feb 2018 18:53:38 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.6 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, FSL_HELO_FAKE, RCVD_IN_DNSWL_HI, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 284C7283D9 for ; Wed, 21 Feb 2018 18:53:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753668AbeBUSxh (ORCPT ); Wed, 21 Feb 2018 13:53:37 -0500 Received: from mail-io0-f193.google.com ([209.85.223.193]:45677 "EHLO mail-io0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751930AbeBUSxe (ORCPT ); Wed, 21 Feb 2018 13:53:34 -0500 Received: by mail-io0-f193.google.com with SMTP id m22so3198468iob.12; Wed, 21 Feb 2018 10:53:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=5dGDA6P+pOkza44X2//FcuKAT/WQ08B5DImGPvVlyPY=; b=ME6FHI+xEuCK1nolmldr9UeGs6UTK8cMjxZjSAT19HhcfS2mdj0Zxbcms2jjg5p2W0 jhfZB1fWnnc1rvmvg06ZgbQymgVJsEsyDzvEpkF+m3ZUINOwhbgauc2UxGj7/jns9xkl P+jkEn1SLHBBzOr3syVQ2Uu3xEl3PX8l6rcphfbWUtQemyxhVxDUIWLpGz6J4beIQ1ja FUdfNIKQwDXem4djXfdsk3SLSzWeioT9TEqpPY7awoC7ERTRVbTqFZo2nHkM1Dqsktzu NzGVt0XBiFBQI65P4esG4/U/U9BXlub9cN6TRqkIRkvRxInyZmePX3qryuSGanwBwP6o RJTg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=5dGDA6P+pOkza44X2//FcuKAT/WQ08B5DImGPvVlyPY=; b=nPPVlAc083n38TGjJlCmHf9UL5QF0T6l0z+rrqkdQeoTOftHHkNs9ykgGguOBW2AD7 QbwXc7WBsHaC3QQbaZxWysZS8lf/qJg7ajgo0qUL4IBlJEabhIjkhcYzoCsQr1otvBC7 L/JJhgbR4l9WkeqwzAVL0T7Xl7QiqId3ypiKonQ2CnrDAucl02Ry9VpOIM6SDLGVCp6J g1wvxxlydTkRQnDYHlQpxvj+7ld/xWO/aKCwWs675wuXKuzW5uWjhQXVCF7lFFICWoWY wL2iKxufVNpfGZYLYjRaFcHLLVwRz+ebcs5T5mI3ERvyAII68WdPhrCD9T11tIZ09Fie 14uQ== X-Gm-Message-State: APf1xPA2BPjKIX/svjDZS2EGiXvokyxvEi4dT6k8adPmLVpPGvT7o4Wu rPxCx2TW1xDoz8zklKeFzbHLnAUL X-Google-Smtp-Source: AG47ELs33jiCj4AYFhIiNj/2bgsPsgPcWv5rvYgOR59IyNx/c3UeE2LYTU9DemgzgLzvGZsZcv3q+Q== X-Received: by 10.107.181.133 with SMTP id e127mr5609471iof.243.1519239213933; Wed, 21 Feb 2018 10:53:33 -0800 (PST) Received: from gmail.com ([2620:15c:17:3:dc28:5c82:b905:e8a8]) by smtp.gmail.com with ESMTPSA id p7sm14973628iof.79.2018.02.21.10.53.33 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 21 Feb 2018 10:53:33 -0800 (PST) Date: Wed, 21 Feb 2018 10:53:31 -0800 From: Eric Biggers To: Chandan Rajendra Cc: linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-fscrypt@vger.kernel.org, tytso@mit.edu Subject: Re: [RFC PATCH V2 10/11] Enable writing encrypted files in blocksize less than pagesize setup Message-ID: <20180221185331.GA114620@gmail.com> References: <20180212094347.22071-1-chandan@linux.vnet.ibm.com> <20180212094347.22071-11-chandan@linux.vnet.ibm.com> <20180221005454.GB252219@gmail.com> <2490066.ZFX8CK6sZb@localhost.localdomain> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <2490066.ZFX8CK6sZb@localhost.localdomain> User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-fscrypt-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fscrypt@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Feb 21, 2018 at 03:27:29PM +0530, Chandan Rajendra wrote: > On Wednesday, February 21, 2018 6:24:54 AM IST Eric Biggers wrote: > > On Mon, Feb 12, 2018 at 03:13:46PM +0530, Chandan Rajendra wrote: > > > This commit splits the functionality of fscrypt_encrypt_block(). The > > > allocation of fscrypt context and cipher text page is moved to a new > > > function fscrypt_prep_ciphertext_page(). > > > > > > ext4_bio_write_page() is modified to appropriately make use of the above > > > two functions. > > > > > > Signed-off-by: Chandan Rajendra > > > > Well, this patch also modifies ext4_bio_write_page() to support the blocksize < > > pagesize case. The commit message makes it sound like it's just refactoring. > > > > > diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c > > > index 0a4a1e7..1e869d5 100644 > > > --- a/fs/ext4/page-io.c > > > +++ b/fs/ext4/page-io.c > > > @@ -419,9 +419,12 @@ int ext4_bio_write_page(struct ext4_io_submit *io, > > > struct inode *inode = page->mapping->host; > > > unsigned block_start; > > > struct buffer_head *bh, *head; > > > + u64 blk_nr; > > > + gfp_t gfp_flags = GFP_NOFS; > > > int ret = 0; > > > int nr_submitted = 0; > > > int nr_to_submit = 0; > > > + int blocksize = (1 << inode->i_blkbits); > > > > > > BUG_ON(!PageLocked(page)); > > > BUG_ON(PageWriteback(page)); > > > @@ -475,15 +478,11 @@ int ext4_bio_write_page(struct ext4_io_submit *io, > > > nr_to_submit++; > > > } while ((bh = bh->b_this_page) != head); > > > > > > - bh = head = page_buffers(page); > > > - > > > - if (ext4_encrypted_inode(inode) && S_ISREG(inode->i_mode) && > > > - nr_to_submit) { > > > - gfp_t gfp_flags = GFP_NOFS; > > > - > > > - retry_encrypt: > > > - data_page = fscrypt_encrypt_block(inode, page, PAGE_SIZE, 0, > > > - page->index, gfp_flags); > > > + if (ext4_encrypted_inode(inode) && S_ISREG(inode->i_mode) > > > + && nr_to_submit) { > > > + retry_prep_ciphertext_page: > > > + data_page = fscrypt_prep_ciphertext_page(inode, page, > > > + gfp_flags); > > > if (IS_ERR(data_page)) { > > > ret = PTR_ERR(data_page); > > > if (ret == -ENOMEM && wbc->sync_mode == WB_SYNC_ALL) { > > > @@ -492,17 +491,28 @@ int ext4_bio_write_page(struct ext4_io_submit *io, > > > congestion_wait(BLK_RW_ASYNC, HZ/50); > > > } > > > gfp_flags |= __GFP_NOFAIL; > > > - goto retry_encrypt; > > > + goto retry_prep_ciphertext_page; > > > } > > > data_page = NULL; > > > goto out; > > > } > > > } > > > > > > + blk_nr = page->index << (PAGE_SHIFT - inode->i_blkbits); > > > + > > > /* Now submit buffers to write */ > > > + bh = head = page_buffers(page); > > > do { > > > if (!buffer_async_write(bh)) > > > continue; > > > + > > > + if (ext4_encrypted_inode(inode) && S_ISREG(inode->i_mode)) { > > > + ret = fscrypt_encrypt_block(inode, page, data_page, blocksize, > > > + bh_offset(bh), blk_nr, gfp_flags); > > > + if (ret) > > > + break; > > > + } > > > + > > > ret = io_submit_add_bh(io, inode, > > > data_page ? data_page : page, bh); > > > if (ret) { > > > @@ -515,12 +525,12 @@ int ext4_bio_write_page(struct ext4_io_submit *io, > > > } > > > nr_submitted++; > > > clear_buffer_dirty(bh); > > > - } while ((bh = bh->b_this_page) != head); > > > + } while (++blk_nr, (bh = bh->b_this_page) != head); > > > > > > /* Error stopped previous loop? Clean up buffers... */ > > > if (ret) { > > > out: > > > - if (data_page) > > > + if (data_page && bh == head) > > > fscrypt_restore_control_page(data_page); > > > printk_ratelimited(KERN_ERR "%s: ret = %d\n", __func__, ret); > > > redirty_page_for_writepage(wbc, page); > > > > I'm wondering why you didn't move the crypto stuff in ext4_bio_write_page() into > > a separate function like I had suggested? It's true we don't have to encrypt > > all the blocks in the page at once, but it would make the crypto stuff more > > self-contained. > > Eric, Are you suggesting that the entire block of code that has invocations to > fscrypt_prep_ciphertext_page() and fscrypt_encrypt_block() be moved to a > separate function that gets defined in fscrypt module? I just had in mind that it would be a separate function in ext4. > > If yes, In Ext4, We have the invocation of io_submit_add_bh() being > interleaved with calls to fscrypt_encrypt_block(). > Well yes that's what your patch does. But we could instead just encrypt all the blocks at once, right? It would be a bit less efficient since we'd have to iterate through the buffer_head list twice, but the advantage is that we end up with ~105 lines ext4_bio_write_page() instead of 130, since the crypto stuff would be more self-contained. Here's an example, given as a diff from master (beware, it's compile-tested only): --- To unsubscribe from this list: send the line "unsubscribe linux-fscrypt" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c index db7590178dfc..e0153c8c4bc4 100644 --- a/fs/ext4/page-io.c +++ b/fs/ext4/page-io.c @@ -409,6 +409,47 @@ static int io_submit_add_bh(struct ext4_io_submit *io, return 0; } +static struct page * +encrypt_page(struct inode *inode, struct page *page, + struct writeback_control *wbc, struct ext4_io_submit *io) +{ + struct page *data_page; + struct buffer_head *bh, *head; + gfp_t gfp_flags = GFP_NOFS; + u64 blk_nr; + int err; + +retry: + data_page = fscrypt_prep_ciphertext_page(inode, page, gfp_flags); + if (IS_ERR(data_page)) + goto out; + + bh = head = page_buffers(page); + blk_nr = (u64)page->index << (PAGE_SHIFT - inode->i_blkbits); + do { + if (!buffer_async_write(bh)) + continue; + err = fscrypt_encrypt_block(inode, page, data_page, bh->b_size, + bh_offset(bh), blk_nr, gfp_flags); + if (err) { + fscrypt_restore_control_page(data_page); + data_page = ERR_PTR(err); + break; + } + } while (blk_nr++, (bh = bh->b_this_page) != head); + +out: + if (data_page == ERR_PTR(-ENOMEM) && wbc->sync_mode == WB_SYNC_ALL) { + if (io->io_bio) { + ext4_io_submit(io); + congestion_wait(BLK_RW_ASYNC, HZ/50); + } + gfp_flags |= __GFP_NOFAIL; + goto retry; + } + return data_page; +} + int ext4_bio_write_page(struct ext4_io_submit *io, struct page *page, int len, @@ -477,23 +518,10 @@ int ext4_bio_write_page(struct ext4_io_submit *io, bh = head = page_buffers(page); - if (ext4_encrypted_inode(inode) && S_ISREG(inode->i_mode) && - nr_to_submit) { - gfp_t gfp_flags = GFP_NOFS; - - retry_encrypt: - data_page = fscrypt_encrypt_page(inode, page, PAGE_SIZE, 0, - page->index, gfp_flags); + if (IS_ENCRYPTED(inode) && S_ISREG(inode->i_mode) && nr_to_submit) { + data_page = encrypt_page(inode, page, wbc, io); if (IS_ERR(data_page)) { ret = PTR_ERR(data_page); - if (ret == -ENOMEM && wbc->sync_mode == WB_SYNC_ALL) { - if (io->io_bio) { - ext4_io_submit(io); - congestion_wait(BLK_RW_ASYNC, HZ/50); - } - gfp_flags |= __GFP_NOFAIL; - goto retry_encrypt; - } data_page = NULL; goto out; }