From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f45.google.com (mail-wr1-f45.google.com [209.85.221.45]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C3C0627FD6D for ; Wed, 25 Mar 2026 14:17:54 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=pass smtp.client-ip=209.85.221.45 ARC-Seal:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774448276; cv=pass; b=JGblT0cw9e/ZrxUTbhHRjWqwb277M6UIl9qN5oyPZYfT6tYT/dZCitdbkYbXGWM35QfaB7tKTaZk8uYzDM22qpU7YDySu1DVkPi/jCObvtujZm+19gAkXbmvN8BoNHkH7feEqq+0i9GoYxR33WOyVT7Zi3BM6JdPQq5DB0qHf38= ARC-Message-Signature:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774448276; c=relaxed/simple; bh=JI+TEE49zQK3cKPmCz6svi6GFaT2s7u6w0MtCQyVKb4=; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject: To:Cc:Content-Type; b=EyOwyeTtns5XSCP8AgC0xyaF7pGCvfeKp+Et8yhRGhUtVNrBzWDSL8ba7Fio1fd0ydkiN0YGMh1fueV349tq4tB3CW4KwH8OmkuiqEnKGIywT76hKcsxGGAasv+6lLGWKUO02nNjU2Mh53UDubEZtnaIhoF0rOPRLEH6ilioIKc= ARC-Authentication-Results:i=2; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com; spf=pass smtp.mailfrom=suse.com; dkim=pass (2048-bit key) header.d=suse.com header.i=@suse.com header.b=He8b1eWY; arc=pass smtp.client-ip=209.85.221.45 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=suse.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=suse.com header.i=@suse.com header.b="He8b1eWY" Received: by mail-wr1-f45.google.com with SMTP id ffacd0b85a97d-43b87970468so1260023f8f.3 for ; Wed, 25 Mar 2026 07:17:54 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1774448273; cv=none; d=google.com; s=arc-20240605; b=Gp2nYfUDKen1bO0TFlciUxJK0m9mpj8v8GBJJUkC0EPKukxgffKP27sqBh44JPo85T AutYkMnnRf7B5CvRhbyic7MFNgFiLGOoDFYUi3F+d0MEKeVB9UmT+rrzOSquDuunPRwj NtFRbFP7mLN2Ubbo88Dpj3B8GxOpuCvKyag2SL01svsvXSIqByTXs4emUOQqbi3hingo kEKb3z7lJCiy5hTAEU0VjpU9CVyqgPSPaNodqkXSpFzB27QB/9KK9aFqKXJ6fyMeaxrV lqb9jx5B/59AaipctEuw2lYoVk+EiB9HtpTMf4F8Zqutariit0JEFWArcCR67qG/PiYf kL9w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:dkim-signature; bh=5n8QXWW6p9E8l82xzEfoMSbTtP2cvmaz+gb8I7NDwzo=; fh=KxUilo+4dGKBgG/c0+AFo1BJgKXjM6Wrm8IxYsg/EII=; b=FUZhIWnSxDQftilhJXlOuTqMWyZA6fVAdnPm6Ix2517vtQwCagZ3GwqKlUYEdPXMBn 7gUJ8VHeaUy/rV6lBbHMvKuhwacPfyvSbyHxhZAdcLuu1Nt7KsADionDQGfPO22MkEWF QYyAJyX2M6vFD6wGseNS728OrSH2Lf9W7f1RfZyqMigSAgYDZsR84RKjEWhwPulg5iR7 LMAPP8lhewysqonWESsq8Tsry6BMXZ6CCSU7A2tZ1UhZiRvt22x0jl8n8K3lyAhn2bMO rRkIXsJbWUdNDN/ZNkksJSpB3DKsYXBjimtg7MNIaGGM3jzOrDKfdA/emjLARzQlCR8L 4rtA==; darn=vger.kernel.org ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1774448273; x=1775053073; darn=vger.kernel.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=5n8QXWW6p9E8l82xzEfoMSbTtP2cvmaz+gb8I7NDwzo=; b=He8b1eWY0jASt4uLuF6oCfL1o9EBtkcoL/seGoFNRov2X0T2vI57qN9LsdTbolUsjd gGdJf7M7Cpzy7/8lRIQmGMlPM2n4D4RRRn4dG8mSEv5WcDNj4ZfrnXfpo7TPheCsTLrn cSiDQVqBLIgxCq/4ndiP4aSrw98h3YOalF/b0D+DhCJEN+2J/w0VCt+5dT6x4xohB+We zeoYEQSoVE4z691qKVDVObHLbxFHW/zIyXLK4VQGTLMTG2oqrSfBjJt/34V4HaUAoxOl 8SYfF9tJnyLr9uxo/Ifuhy5V7mCUdhCpy1Lod4EBRcutAx/cEtrAP29TMps1udKBPcR5 NilQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1774448273; x=1775053073; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=5n8QXWW6p9E8l82xzEfoMSbTtP2cvmaz+gb8I7NDwzo=; b=i5YpQaskz3fwE7hXpJrC7jWBYmwxxbA2WPJ94BiGuM/IDqWFEABMAUaP9IZwMbg16C kQ7V9Ts83fzC45LJENToY2PPh+9SGdToGw1ggfCnmqwaMHl8roXEyNe2GIgbwSwFIKPB eF5uZlhmFxczrQNdREesDF9F0U5HT+BySVk3SJir1DYpQWyJ9WfzU9VJ3lVXjh12jQxr CAQK4kwK9yknecHCkoulIB67TwoaQP14ET1dNSMzztBDD4j2MwhZq7NlxeIPuTs6CYsp 6X7KSQ2NO6AaPSXZ0nQClVskWj/9WEqrqLSVEuX0WTGIc6DXu/FHUJ7lbc/nTCl+T6di wu8A== X-Forwarded-Encrypted: i=1; AJvYcCW+F7rnjwP1j/YWgpCdVuNJZMKfsCbyWYNj80TeqIpmFU7VMTNoSy5GUtn7imK4cIKutSY6ZbN6bn5LfFQ=@vger.kernel.org X-Gm-Message-State: AOJu0YxJmlA50cBgC+Vj4tNPod+4Mmqnbax8euMEu60AVv2jLpHaqbpg QHbP3OIpY91+8LvFyijmWgJbCG9pnlqAoCX+LVAnsXgRKSXmsJ4G7rWlBaeKYRQV81xyA5i0vTl MHKJMC1VZqrVn5H0admOKtDbeL3mM1u4G1eUy0t68bw== X-Gm-Gg: ATEYQzz1FBjXvpcYciEfIMuED+awt81QANJ9GAEEMykFPxqjCptPJdMsvrC7cfPLafY +PqRTbpoCADqpCqZMaosre7y6Ea90beeMjMD7jmk1feBM9UMcFuOWDzhWY+dN03jwpT0J96TvtM 41VWRLP0q4aD6nigfVBaE1BH9OaoLvXOIHJaMZfafediM++IozEBLui+pZ9maIzbF91TyJEPhYX 0LJ/Ow/Awh7x8e6KlOFsx5NbXzBwig3AEWI0WJyy1XtoQ7F8F0+iT3F/LHqYaejwyJeYV1HvTiT PGHTzPWWk8tLKPfcmm+/fkLOhpgJXCWJptYqQK8VR6pn5mIyxDqwXScyZqIzzplMKV/fVEAmAnF npqfE X-Received: by 2002:a05:6000:2084:b0:43b:3f2d:7d3c with SMTP id ffacd0b85a97d-43b88a21758mr5567679f8f.2.1774448273072; Wed, 25 Mar 2026 07:17:53 -0700 (PDT) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 References: <20260206182336.1397715-1-neelx@suse.com> <20260206182336.1397715-34-neelx@suse.com> <20260208150854.3090411-1-clm@meta.com> In-Reply-To: <20260208150854.3090411-1-clm@meta.com> From: Daniel Vacek Date: Wed, 25 Mar 2026 15:17:41 +0100 X-Gm-Features: AQROBzCA2SzXEdLH4PwF7MzsDSpXTmv7zxyVpq6_W8dQnpHa9Q-YU8fXQyJUEFs Message-ID: Subject: Re: [PATCH v6 33/43] btrfs: implement read repair for encryption To: Chris Mason Cc: Chris Mason , Josef Bacik , Eric Biggers , "Theodore Y. Ts'o" , Jaegeuk Kim , Jens Axboe , David Sterba , linux-block@vger.kernel.org, linux-fscrypt@vger.kernel.org, linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" On Sun, 8 Feb 2026 at 16:09, Chris Mason wrote: > Daniel Vacek wrote: > > From: Josef Bacik > > > > In order to do read repair we will allocate sectorsize bio's and read > > them one at a time, repairing any sectors that don't match their csum. > > In order to do this we re-submit the IO's after it's failed, and at this > > point we still need the fscrypt_extent_info for these new bio's. > > > > Add the fscrypt_extent_info to the read part of the union in the > > btrfs_bio, and then pass this through all the places where we do reads. > > Additionally add the orig_start, because we need to be able to put the > > correct extent offset for the encryption context. > > > > With these in place we can utilize the normal read repair path. The > > only exception is that the actual repair of the bad copies has to be > > triggered from the ->process_bio callback, because this is the encrypted > > data. If we waited until the end_io we would have the decrypted data > > and we don't want to write that to the disk. This is the only change to > > the normal read repair path, we trigger the fixup of the broken sectors > > in ->process_bio, and then we skip that part if we successfully repair > > the sector in ->process_bio once we get to the endio. > > Hi everyone, > > I'm running the btrfs for-next branch, along with the encryption patch set > through my AI review prompts: > > https://github.com/masoncl/review-prompts > > I'm working on some btrfs specific instructions, but hopefully these reviews > are already useful. I've kept the headers to make it easier to double check > that I'm actually replying to the right email, but please let me know if any > parts of this are incorrect: > > commit 096c17a6a951ed0c1100e863c4ee3a95bab0ad51 > Author: Josef Bacik > > btrfs: implement read repair for encryption > > This commit implements read repair functionality for encrypted btrfs > files by adding fscrypt_extent_info tracking to the btrfs_bio structure > and handling repair operations before decryption occurs. > > > diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c > > index c28f3f53bad28..e82de96b533cb 100644 > > --- a/fs/btrfs/bio.c > > +++ b/fs/btrfs/bio.c > > [ ... ] > > > @@ -293,6 +323,13 @@ static struct btrfs_failed_bio *repair_one_sector(struct btrfs_bio *failed_bbio, > > repair_bbio = btrfs_bio(repair_bio); > > btrfs_bio_init(repair_bbio, failed_bbio->inode, failed_bbio->file_offset + bio_offset, > > NULL, fbio); > > + repair_bbio->fscrypt_info = fscrypt_get_extent_info(failed_bbio->fscrypt_info); > > + repair_bbio->orig_start = failed_bbio->orig_start; > > + > > + btrfs_set_bio_crypt_ctx_from_extent(repair_bio, repair_bbio->inode, > > + failed_bbio->fscrypt_info, > > + repair_bbio->file_offset - > > + failed_bbio->orig_start); > > > > mirror = next_repair_mirror(fbio, failed_bbio->mirror_num); > > btrfs_debug(fs_info, "submitting repair read to mirror %d", mirror); > > [ ... ] > > > @@ -214,17 +244,17 @@ static void btrfs_end_repair_bio(struct btrfs_bio *repair_bbio, > > goto done; > > } > > > > + btrfs_set_bio_crypt_ctx_from_extent(&repair_bbio->bio, > > + repair_bbio->inode, > > + repair_bbio->fscrypt_info, > > + repair_bbio->file_offset - > > + repair_bbio->orig_start); > > + > > btrfs_submit_bbio(repair_bbio, mirror); > > return; > > } > > > > - do { > > - mirror = prev_repair_mirror(fbio, mirror); > > - btrfs_repair_io_failure(fs_info, btrfs_ino(inode), > > - repair_bbio->file_offset, fs_info->sectorsize, > > - logical, paddrs, step, mirror); > > - } while (mirror != fbio->bbio->mirror_num); > > - > > + handle_repair(repair_bbio, paddrs); > > done: > > btrfs_repair_done(fbio); > > bio_put(&repair_bbio->bio); > > Does this leak the fscrypt_extent_info reference acquired at line 326? > In repair_one_sector(), repair_bbio->fscrypt_info is set via > fscrypt_get_extent_info(failed_bbio->fscrypt_info), but when > btrfs_end_repair_bio() completes at the done label, it calls > bio_put(&repair_bbio->bio) without calling > fscrypt_put_extent_info(repair_bbio->fscrypt_info) first. repair_one_sector() is called from btrfs_check_read_bio() which always has (bbio a.k.a. failed_bbio)->fscrypt_info NULL. So there is no object which could leak due to refcounting imbalance. Encrypted read bbio passes through btrfs_check_encrypted_read_bio() instead which does not yet implement repair. Moreover btrfs_end_repair_bio() is called before repair_one_sector(). And when it is called btrfs_check_read_bio() returns and no repair_one_sector() is caled at all. The fscrypt_put_extent_info() is eventually called from btrfs_simple_end_io() => simple_end_io_work() => btrfs_bio_end_io(). So this looks clean, unless I missed something. > > diff --git a/fs/btrfs/bio.h b/fs/btrfs/bio.h > > index 456d32db9e9e9..7a8ff4378cba0 100644 > > --- a/fs/btrfs/bio.h > > +++ b/fs/btrfs/bio.h > > [ ... ] > > > diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c > > index 5a39f707c2933..afc461d7642e4 100644 > > --- a/fs/btrfs/compression.c > > +++ b/fs/btrfs/compression.c > > @@ -562,6 +562,8 @@ void btrfs_submit_compressed_read(struct btrfs_bio *bbio) > > cb->compress_type = btrfs_extent_map_compression(em); > > cb->orig_bbio = bbio; > > cb->bbio.csum_search_commit_root = bbio->csum_search_commit_root; > > + cb->bbio.fscrypt_info = fscrypt_get_extent_info(em->fscrypt_info); > > + cb->bbio.orig_start = 0; > > > > btrfs_set_bio_crypt_ctx_from_extent(&cb->bbio.bio, inode, em->fscrypt_info, 0); > > btrfs_free_extent_map(em); > > Does this leak the fscrypt_extent_info reference in the compressed read > path? At line 565, fscrypt_get_extent_info() acquires a reference and > stores it in cb->bbio.fscrypt_info. Looking at end_bbio_compressed_read() > in compression.c, it calls btrfs_bio_end_io(cb->orig_bbio, status) which > releases the ORIGINAL bbio's fscrypt_info, but then calls bio_put(cb_bio) > without calling fscrypt_put_extent_info(cb->bbio.fscrypt_info). The error > path in cleanup_compressed_bio() also calls bio_put() without releasing > the cb->bbio.fscrypt_info reference. IIUC, the fscrypt_info is put in btrfs_bio_end_io() before calling end_bbio_compressed_read(). So again, this looks clean to me. --nX > [ ... ] > >