Message ID | 20230525214822.2725616-1-kent.overstreet@linux.dev |
---|---|
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp65220vqr; Thu, 25 May 2023 14:51:19 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ5qlUYysVkCI5zActAL4SLMC0vG3SKRUuasqb7XCcebneeCmMl/mbEDVXnCDkCZyVXJ/oJe X-Received: by 2002:a05:6a00:21d4:b0:644:18fe:91cc with SMTP id t20-20020a056a0021d400b0064418fe91ccmr196179pfj.12.1685051479456; Thu, 25 May 2023 14:51:19 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1685051479; cv=none; d=google.com; s=arc-20160816; b=nNh7d3rQhs+0Ouc27mAsU3wVhBRYeBuG8mx/tT5yzmzqsVcQugCbnLobctlP7OjkJC NzRAPDzlVvAz+QWHHLB3TnXVDTqNfhQr5n8W0bw9DDaPJX834SUNlcBleN5hOQxRd8gg mLnpkbKtRo2kuhtRB3LMkf0RJLokbpR4bWHmMdEerEkqrgOZexsEtCjaGnx1n7CvmdrM kfanoDBg5tVIJcUnOWagHOS7gnbcjb+l0AMdoJfL94c99ivk/Pq/CQqF4uvJlCqtgcxO qt20ku/xjZXwC2S67X8OpbeidCWXYTvJFdlym13MK/YHw6fcEpDrfLsRP198oUUiqTMB PBbQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=/htzEy4EHBMdNE/JbpSxK/E6aimlWCZ120WLz6hVPdg=; b=WVewXHvYMdlSo5QiV1AIfIGf0nxzUemBQWeTvUcronJIz3yDiydpLCxhBxMRbug4yj M3X7h/BRZQTaxcnCg4jCbtP/4+8wjnAAOFnCew6/lyFIDuSajdydKxddt4RoMNtvsMtH 3HTQLq9l6CRqBMAEWhfISYRtpNlPXxrOvoxfejoCCrBkaidW2FNOVOzZ32CL7zB1XFs+ ZlhKtUAKGLztOKiIo1L4n/NMZt793wCL3OQeUVlg+DvEQSDooZUohW2pl29TcgLgfQgs kDZuALOrOifw9D99CfvivUqRPIuXBQ+MvqdmxuS8rPIOv2bNKmagR8f3/7jKmz5NFzxV pyHQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linux.dev header.s=key1 header.b=k6jGFcUq; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linux.dev Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id i5-20020a056a00004500b0062d7d3c6cadsi2360894pfk.333.2023.05.25.14.51.05; Thu, 25 May 2023 14:51:19 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@linux.dev header.s=key1 header.b=k6jGFcUq; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linux.dev Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241255AbjEYVsp (ORCPT <rfc822;jian.xie.xdx@gmail.com> + 99 others); Thu, 25 May 2023 17:48:45 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50860 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230203AbjEYVsk (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 25 May 2023 17:48:40 -0400 Received: from out-56.mta1.migadu.com (out-56.mta1.migadu.com [95.215.58.56]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4613299 for <linux-kernel@vger.kernel.org>; Thu, 25 May 2023 14:48:39 -0700 (PDT) X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1685051317; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=/htzEy4EHBMdNE/JbpSxK/E6aimlWCZ120WLz6hVPdg=; b=k6jGFcUqjzohjB8xz2f7m8Jps7CXkyBO6+5Ta9qstxqh0NLeOgn367GHD3EQsk1sgmfZKd DPMXEQ/RFaJQFONq4eZEvQUZC/bfUO7wHYP8CqKmiZDQNXvb5ia9htTi5KjE5B/dPmXp8/ m6IgFfuoGt5vQIdcp87lhi+vrPF8uxU= From: Kent Overstreet <kent.overstreet@linux.dev> To: linux-kernel@vger.kernel.org, axboe@kernel.dk Cc: Kent Overstreet <kent.overstreet@linux.dev>, linux-block@vger.kernel.org, linux-fsdevel@vger.kernel.org Subject: [PATCH 0/7] block layer patches for bcachefs Date: Thu, 25 May 2023 17:48:15 -0400 Message-Id: <20230525214822.2725616-1-kent.overstreet@linux.dev> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS, T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1766904540186835069?= X-GMAIL-MSGID: =?utf-8?q?1766904540186835069?= |
Series |
block layer patches for bcachefs
|
|
Message
Kent Overstreet
May 25, 2023, 9:48 p.m. UTC
Jens, here's the full series of block layer patches needed for bcachefs: Some of these (added exports, zero_fill_bio_iter?) can probably go with the bcachefs pull and I'm just including here for completeness. The main ones are the bio_iter patches, and the __invalidate_super() patch. The bio_iter series has a new documentation patch. I would still like the __invalidate_super() patch to get some review (from VFS people? unclear who owns this). Thanks, Kent Kent Overstreet (7): block: Add some exports for bcachefs block: Allow bio_iov_iter_get_pages() with bio->bi_bdev unset block: Bring back zero_fill_bio_iter block: Rework bio_for_each_segment_all() block: Rework bio_for_each_folio_all() block: Add documentation for bio iterator macros block: Don't block on s_umount from __invalidate_super() block/bdev.c | 2 +- block/bio.c | 57 ++++++------ block/blk-core.c | 1 + block/blk-map.c | 38 ++++---- block/blk.h | 1 - block/bounce.c | 12 +-- drivers/md/bcache/btree.c | 8 +- drivers/md/dm-crypt.c | 10 +- drivers/md/raid1.c | 4 +- fs/btrfs/disk-io.c | 4 +- fs/btrfs/extent_io.c | 50 +++++----- fs/btrfs/raid56.c | 14 +-- fs/crypto/bio.c | 9 +- fs/erofs/zdata.c | 4 +- fs/ext4/page-io.c | 8 +- fs/ext4/readpage.c | 4 +- fs/f2fs/data.c | 20 ++-- fs/gfs2/lops.c | 10 +- fs/gfs2/meta_io.c | 8 +- fs/iomap/buffered-io.c | 14 +-- fs/mpage.c | 4 +- fs/squashfs/block.c | 48 +++++----- fs/squashfs/lz4_wrapper.c | 17 ++-- fs/squashfs/lzo_wrapper.c | 17 ++-- fs/squashfs/xz_wrapper.c | 19 ++-- fs/squashfs/zlib_wrapper.c | 18 ++-- fs/squashfs/zstd_wrapper.c | 19 ++-- fs/super.c | 40 ++++++-- fs/verity/verify.c | 9 +- include/linux/bio.h | 186 +++++++++++++++++++++++++------------ include/linux/blkdev.h | 1 + include/linux/bvec.h | 70 ++++++++------ include/linux/fs.h | 1 + 33 files changed, 429 insertions(+), 298 deletions(-)
Comments
On 5/25/23 3:48 PM, Kent Overstreet wrote: > Jens, here's the full series of block layer patches needed for bcachefs: > > Some of these (added exports, zero_fill_bio_iter?) can probably go with > the bcachefs pull and I'm just including here for completeness. The main > ones are the bio_iter patches, and the __invalidate_super() patch. > > The bio_iter series has a new documentation patch. > > I would still like the __invalidate_super() patch to get some review > (from VFS people? unclear who owns this). I wanted to check the code generation for patches 4 and 5, but the series doesn't seem to apply to current -git nor my for-6.5/block. There's no base commit in this cover letter either, so what is this against? Please send one that applies to for-6.5/block so it's a bit easier to take a closer look at this.
On Fri, May 26, 2023 at 08:35:23AM -0600, Jens Axboe wrote: > On 5/25/23 3:48 PM, Kent Overstreet wrote: > > Jens, here's the full series of block layer patches needed for bcachefs: > > > > Some of these (added exports, zero_fill_bio_iter?) can probably go with > > the bcachefs pull and I'm just including here for completeness. The main > > ones are the bio_iter patches, and the __invalidate_super() patch. > > > > The bio_iter series has a new documentation patch. > > > > I would still like the __invalidate_super() patch to get some review > > (from VFS people? unclear who owns this). > > I wanted to check the code generation for patches 4 and 5, but the > series doesn't seem to apply to current -git nor my for-6.5/block. > There's no base commit in this cover letter either, so what is this > against? > > Please send one that applies to for-6.5/block so it's a bit easier > to take a closer look at this. Here you go: git pull https://evilpiepirate.org/git/bcachefs.git block-for-bcachefs changed folio_vec to folio_seg, build tested it and just pointed my CI at it, results will be showing up for xfs at https://evilpiepirate.org/~testdashboard/ci?branch=block-for-bcachefs
On 5/26/23 2:44?PM, Kent Overstreet wrote: > On Fri, May 26, 2023 at 08:35:23AM -0600, Jens Axboe wrote: >> On 5/25/23 3:48?PM, Kent Overstreet wrote: >>> Jens, here's the full series of block layer patches needed for bcachefs: >>> >>> Some of these (added exports, zero_fill_bio_iter?) can probably go with >>> the bcachefs pull and I'm just including here for completeness. The main >>> ones are the bio_iter patches, and the __invalidate_super() patch. >>> >>> The bio_iter series has a new documentation patch. >>> >>> I would still like the __invalidate_super() patch to get some review >>> (from VFS people? unclear who owns this). >> >> I wanted to check the code generation for patches 4 and 5, but the >> series doesn't seem to apply to current -git nor my for-6.5/block. >> There's no base commit in this cover letter either, so what is this >> against? >> >> Please send one that applies to for-6.5/block so it's a bit easier >> to take a closer look at this. > > Here you go: > git pull https://evilpiepirate.org/git/bcachefs.git block-for-bcachefs Thanks The re-exporting of helpers is somewhat odd - why is bcachefs special here and needs these, while others do not? But the main issue for me are the iterator changes, which mostly just seems like unnecessary churn. What's the justification for these? The commit messages don;t really have any. Doesn't seem like much of a simplification, and in fact it's more code than before and obviously more stack usage as well.
On Tue, May 30, 2023 at 08:22:50AM -0600, Jens Axboe wrote: > On 5/26/23 2:44?PM, Kent Overstreet wrote: > > On Fri, May 26, 2023 at 08:35:23AM -0600, Jens Axboe wrote: > >> On 5/25/23 3:48?PM, Kent Overstreet wrote: > >>> Jens, here's the full series of block layer patches needed for bcachefs: > >>> > >>> Some of these (added exports, zero_fill_bio_iter?) can probably go with > >>> the bcachefs pull and I'm just including here for completeness. The main > >>> ones are the bio_iter patches, and the __invalidate_super() patch. > >>> > >>> The bio_iter series has a new documentation patch. > >>> > >>> I would still like the __invalidate_super() patch to get some review > >>> (from VFS people? unclear who owns this). > >> > >> I wanted to check the code generation for patches 4 and 5, but the > >> series doesn't seem to apply to current -git nor my for-6.5/block. > >> There's no base commit in this cover letter either, so what is this > >> against? > >> > >> Please send one that applies to for-6.5/block so it's a bit easier > >> to take a closer look at this. > > > > Here you go: > > git pull https://evilpiepirate.org/git/bcachefs.git block-for-bcachefs > > Thanks > > The re-exporting of helpers is somewhat odd - why is bcachefs special > here and needs these, while others do not? It's not iomap based. > But the main issue for me are the iterator changes, which mostly just > seems like unnecessary churn. What's the justification for these? The > commit messages don;t really have any. Doesn't seem like much of a > simplification, and in fact it's more code than before and obviously > more stack usage as well. I need bio_for_each_folio(). The approach taken by the bcachefs IO paths is to first build up bios, then walk the extents btree to determine where to send them, splitting as needed. For reading into the page cache we additionally need to initialize our private state based on what we're reading from that says what's on disk (unallocated, reservation, or normal allocation) and how many replicas. This is used for both i_blocks accounting and for deciding when we need to get a disk reservation. Since we're doing this post split, it needs bio_for_each_folio, not the _all variant. Yes, the iterator changes are a bit more code - but it's split up into better helpers now, the pointer arithmetic before was a bit dense; I found the result to be more readable. I'm surprised at more stack usage; I would have expected _less_ for bio_for_each_page_all() since it gets rid of a pointer into the bvec_iter_all. How did you measure that?
On 5/30/23 10:06?AM, Kent Overstreet wrote: > On Tue, May 30, 2023 at 08:22:50AM -0600, Jens Axboe wrote: >> On 5/26/23 2:44?PM, Kent Overstreet wrote: >>> On Fri, May 26, 2023 at 08:35:23AM -0600, Jens Axboe wrote: >>>> On 5/25/23 3:48?PM, Kent Overstreet wrote: >>>>> Jens, here's the full series of block layer patches needed for bcachefs: >>>>> >>>>> Some of these (added exports, zero_fill_bio_iter?) can probably go with >>>>> the bcachefs pull and I'm just including here for completeness. The main >>>>> ones are the bio_iter patches, and the __invalidate_super() patch. >>>>> >>>>> The bio_iter series has a new documentation patch. >>>>> >>>>> I would still like the __invalidate_super() patch to get some review >>>>> (from VFS people? unclear who owns this). >>>> >>>> I wanted to check the code generation for patches 4 and 5, but the >>>> series doesn't seem to apply to current -git nor my for-6.5/block. >>>> There's no base commit in this cover letter either, so what is this >>>> against? >>>> >>>> Please send one that applies to for-6.5/block so it's a bit easier >>>> to take a closer look at this. >>> >>> Here you go: >>> git pull https://evilpiepirate.org/git/bcachefs.git block-for-bcachefs >> >> Thanks >> >> The re-exporting of helpers is somewhat odd - why is bcachefs special >> here and needs these, while others do not? > > It's not iomap based. > >> But the main issue for me are the iterator changes, which mostly just >> seems like unnecessary churn. What's the justification for these? The >> commit messages don;t really have any. Doesn't seem like much of a >> simplification, and in fact it's more code than before and obviously >> more stack usage as well. > > I need bio_for_each_folio(). > > The approach taken by the bcachefs IO paths is to first build up bios, > then walk the extents btree to determine where to send them, splitting > as needed. > > For reading into the page cache we additionally need to initialize our > private state based on what we're reading from that says what's on > disk (unallocated, reservation, or normal allocation) and how many > replicas. This is used for both i_blocks accounting and for deciding > when we need to get a disk reservation. Since we're doing this post > split, it needs bio_for_each_folio, not the _all variant. > > Yes, the iterator changes are a bit more code - but it's split up into > better helpers now, the pointer arithmetic before was a bit dense; I > found the result to be more readable. I'm surprised at more stack > usage; I would have expected _less_ for bio_for_each_page_all() since > it gets rid of a pointer into the bvec_iter_all. How did you measure > that? Sorry typo, I meant text. Just checked stack and it looks identical, but things like blk-map grows ~6% more text, and bio ~3%. Didn't check all of them, but at least those two are consistent across x86-64 and aarch64. Ditto on the data front. Need to take a closer look at where exactly that is coming from, and what that looks like.
On Tue, May 30, 2023 at 10:50:55AM -0600, Jens Axboe wrote: > Sorry typo, I meant text. Just checked stack and it looks identical, but > things like blk-map grows ~6% more text, and bio ~3%. Didn't check all > of them, but at least those two are consistent across x86-64 and > aarch64. Ditto on the data front. Need to take a closer look at where > exactly that is coming from, and what that looks like. Weird - when I looked kernel text size was unchanged, it was data that increased with the earlier version of the patch due to a new WARN_ON(). I'll have another look.
On Tue, May 30, 2023 at 10:50:55AM -0600, Jens Axboe wrote: > Sorry typo, I meant text. Just checked stack and it looks identical, but > things like blk-map grows ~6% more text, and bio ~3%. Didn't check all > of them, but at least those two are consistent across x86-64 and > aarch64. Ditto on the data front. Need to take a closer look at where > exactly that is coming from, and what that looks like. A good chunk of that is because I added warnings and assertions for e.g. running past the end of the bvec array. These bugs are rare and shouldn't happen with normal iterator usage (e.g. the bio_for_each_* macros), but I'd like to keep them as a debug mode thing. But we don't yet have CONFIG_BLOCK_DEBUG - perhaps we should. With those out, I see a code size decrease in bio.c, which makes sense - gcc ought to be able to generate slightly better code when it's dealing with pure values, provided everything is inlined and there's no aliasing considerations. Onto blk-map.c: bio_copy_kern_endio_read() increases in code size, but if I change memcpy_from_bvec() to take the bvec by val instead of by ref it's basically the same code size. There's no disadvantage to changing memcpy_from_bvec() to pass by val. bio_copy_(to|from)_iter() is a wtf, though - gcc is now spilling the constructed bvec to the stack; my best guess is it's a register pressure thing (but we shouldn't be short registers here!). So, since the fastpath stuff in bio.c gets smaller and blk-map.c is not exactly fastpath stuff I'm not inclined to fight with gcc on this one - let me know if that works for you. Branch is updated - I split out the new assertions into a separate patch that adds CONFIG_BLK_DEBUG, and another patch for mempcy_(to|from)_bio() for a small code size decrease. https://evilpiepirate.org/git/bcachefs.git/log/?h=block-for-bcachefs or git pull http://evilpiepirate.org/git/bcachefs.git block-for-bcachefs
On 6/4/23 5:38?PM, Kent Overstreet wrote: > On Tue, May 30, 2023 at 10:50:55AM -0600, Jens Axboe wrote: >> Sorry typo, I meant text. Just checked stack and it looks identical, but >> things like blk-map grows ~6% more text, and bio ~3%. Didn't check all >> of them, but at least those two are consistent across x86-64 and >> aarch64. Ditto on the data front. Need to take a closer look at where >> exactly that is coming from, and what that looks like. > > A good chunk of that is because I added warnings and assertions for > e.g. running past the end of the bvec array. These bugs are rare and > shouldn't happen with normal iterator usage (e.g. the bio_for_each_* > macros), but I'd like to keep them as a debug mode thing. > > But we don't yet have CONFIG_BLOCK_DEBUG - perhaps we should. Let's split those out then, especially as we don't have a BLOCK_DEBUG option right now. > With those out, I see a code size decrease in bio.c, which makes sense - > gcc ought to be able to generate slightly better code when it's dealing > with pure values, provided everything is inlined and there's no aliasing > considerations. > > Onto blk-map.c: > > bio_copy_kern_endio_read() increases in code size, but if I change > memcpy_from_bvec() to take the bvec by val instead of by ref it's > basically the same code size. There's no disadvantage to changing > memcpy_from_bvec() to pass by val. > > bio_copy_(to|from)_iter() is a wtf, though - gcc is now spilling the > constructed bvec to the stack; my best guess is it's a register pressure > thing (but we shouldn't be short registers here!). > > So, since the fastpath stuff in bio.c gets smaller and blk-map.c is not > exactly fastpath stuff I'm not inclined to fight with gcc on this one - > let me know if that works for you. > > Branch is updated - I split out the new assertions into a separate patch > that adds CONFIG_BLK_DEBUG, and another patch for mempcy_(to|from)_bio() > for a small code size decrease. > > https://evilpiepirate.org/git/bcachefs.git/log/?h=block-for-bcachefs > or > git pull http://evilpiepirate.org/git/bcachefs.git block-for-bcachefs Cn you resend just the iterator changes in their current form? The various re-exports are a separate discussion, I think we should focus on the iterator bits first.
On Mon, Jun 05, 2023 at 10:49:37AM -0600, Jens Axboe wrote: > On 6/4/23 5:38?PM, Kent Overstreet wrote: > > On Tue, May 30, 2023 at 10:50:55AM -0600, Jens Axboe wrote: > >> Sorry typo, I meant text. Just checked stack and it looks identical, but > >> things like blk-map grows ~6% more text, and bio ~3%. Didn't check all > >> of them, but at least those two are consistent across x86-64 and > >> aarch64. Ditto on the data front. Need to take a closer look at where > >> exactly that is coming from, and what that looks like. > > > > A good chunk of that is because I added warnings and assertions for > > e.g. running past the end of the bvec array. These bugs are rare and > > shouldn't happen with normal iterator usage (e.g. the bio_for_each_* > > macros), but I'd like to keep them as a debug mode thing. > > > > But we don't yet have CONFIG_BLOCK_DEBUG - perhaps we should. > > Let's split those out then, especially as we don't have a BLOCK_DEBUG > option right now. Already did that; there's a patch in the branch that adds CONFIG_BLK_DEBUG with the new assertions. > Cn you resend just the iterator changes in their current form? The > various re-exports are a separate discussion, I think we should focus on > the iterator bits first. They're up in that branch with the iterator changes first now; I'll mail them out too.