On Sun, 8 Feb 2026 at 16:09, Chris Mason <[email protected]> wrote: > Daniel Vacek <[email protected]> wrote: > > From: Josef Bacik <[email protected]> > > > > 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 <[email protected]> > > 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 > [ ... ] > >
