Re: [PATCH 24/79] block: rust: add `BadBlocks` for bad block tracking

[Andreas Hindborg <a.hindborg@kernel.org>]

Alice Ryhl <aliceryhl@google.com> writes:

> On Mon, Feb 16, 2026 at 12:35:11AM +0100, Andreas Hindborg wrote:
>> Add a safe Rust wrapper around the Linux kernel's badblocks infrastructure
>> to track and manage defective sectors on block devices. The BadBlocks type
>> provides methods to:
>> 
>> - Mark sectors as bad or good (set_bad/set_good)
>> - Check if sector ranges contain bad blocks (check)
>> - Automatically handle memory management with PinnedDrop
>> 
>> The implementation includes comprehensive documentation with examples for
>> block device drivers that need to avoid known bad sectors to maintain
>> data integrity. Bad blocks information is used by device drivers,
>> filesystem layers, and device management tools.
>> 
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>
>> diff --git a/rust/kernel/block/badblocks.rs b/rust/kernel/block/badblocks.rs
>> new file mode 100644
>> index 0000000000000..a5fe0fde2e755
>> --- /dev/null
>> +++ b/rust/kernel/block/badblocks.rs
>> @@ -0,0 +1,721 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Bad blocks tracking for block devices.
>> +//!
>> +//! This module provides a safe Rust wrapper around the badblocks
>> +//! infrastructure, which is used to track and manage bad sectors on block
>> +//! devices. Bad blocks are sectors that cannot reliably store data and should
>> +//! be avoided during I/O operations.
>
> Could use a srctree link to badblocks.h here.

Will add.

>
>> +/// # Examples
>> +///
>> +/// Basic usage:
>> +///
>> +/// ```rust
>> +/// # use kernel::block::badblocks::{BadBlocks, BlockStatus};
>> +/// # use kernel::prelude::*;
>> +///
>
> Unhide the imports or remove this empty line.

Ok.

>
>> +/// // Create a new bad blocks tracker
>> +/// let bad_blocks = KBox::pin_init(BadBlocks::new(true), GFP_KERNEL)?;
>> +///
>> +/// // Mark sectors 100-109 as bad (unacknowledged)
>> +/// bad_blocks.set_bad(100..110, false)?;
>> +///
>> +/// // Check if sector range 95-104 contains bad blocks
>> +/// match bad_blocks.check(95..105) {
>> +///     BlockStatus::None => pr_info!("No bad blocks found"),
>> +///     BlockStatus::Acknowledged(range) => pr_warn!("Acknowledged bad blocks: {:?}", range),
>> +///     BlockStatus::Unacknowledged(range) => pr_err!("Unacknowledged bad blocks: {:?}", range),
>> +/// }
>> +/// # Ok::<(), kernel::error::Error>(())
>> +/// ```
>> +/// # Invariants
>> +///
>> +/// - `self.blocks` is a valid `bindings::badblocks` struct.
>> +#[pin_data(PinnedDrop)]
>> +pub struct BadBlocks {
>> +    #[pin]
>> +    blocks: Opaque<bindings::badblocks>,
>> +}
>> +
>> +impl BadBlocks {
>> +    /// Creates a new bad blocks tracker.
>> +    ///
>> +    /// Initializes an empty bad blocks tracker that can manage defective sectors
>> +    /// on a block device. The tracker starts with no bad blocks recorded and
>> +    /// allocates a single page for storing bad block entries.
>> +    ///
>> +    /// # Returns
>> +    ///
>> +    /// Returns a [`PinInit`] that can be used to initialize a [`BadBlocks`] instance.
>> +    /// Initialization may fail with `ENOMEM` if memory allocation fails.
>> +    ///
>> +    /// # Examples
>> +    ///
>> +    /// ```rust
>> +    /// # use kernel::block::badblocks::{BadBlocks, BlockStatus};
>> +    /// # use kernel::prelude::*;
>> +    ///
>
> Ditto. (Many times throughout file.)
>
>> +    /// // Create and initialize a bad blocks tracker
>> +    /// let bad_blocks = KBox::pin_init(BadBlocks::new(true), GFP_KERNEL)?;
>> +    ///
>> +    /// // The tracker is ready to use with no bad blocks initially
>> +    /// match bad_blocks.check(0..100) {
>> +    ///     BlockStatus::None => pr_info!("No bad blocks found initially"),
>> +    ///     _ => unreachable!(),
>> +    /// }
>> +    /// # Ok::<(), kernel::error::Error>(())
>> +    /// ```
>> +    pub fn new(enable: bool) -> impl PinInit<Self, Error> {
>> +        // INVARIANT: We initialize `self.blocks` below. If initialization fails, an error is
>> +        // returned.
>> +        try_pin_init!(Self {
>> +            blocks <- Opaque::try_ffi_init(|slot| {
>> +                // SAFETY: `slot` is a valid pointer to uninitialized memory
>> +                // allocated by the Opaque type. `badblocks_init` is safe to
>> +                // call with uninitialized memory.
>> +                to_result(unsafe {bindings::badblocks_init(slot, if enable {1} else {0})})
>
> I think you can just cast the boolean to an integer.

Ok.

>
> Also, formatting here is off (but ignored by rustfmt due to macro.)

Will try to fix.

>
>> +    /// Enables the bad blocks tracker if it was previously disabled.
>> +    ///
>> +    /// Attempts to enable bad block tracking by transitioning the tracker from
>> +    /// a disabled state to an enabled state.
>> +    ///
>> +    /// # Behavior
>> +    ///
>> +    /// - If the tracker is disabled, it will be enabled.
>> +    /// - If the tracker is already enabled, this operation has no effect.
>> +    /// - The operation is atomic and thread-safe.
>> +    ///
>> +    /// # Usage
>> +    ///
>> +    /// Bad blocks trackers can be created in a disabled state and enabled later
>> +    /// when needed. This is useful for conditional bad block tracking or for
>> +    /// deferring activation until the device is fully initialized.
>> +    ///
>> +    /// # Examples
>> +    ///
>> +    /// ```rust
>> +    /// # use kernel::block::badblocks::BadBlocks;
>> +    /// # use kernel::prelude::*;
>> +    ///
>> +    /// // Create a disabled bad blocks tracker
>> +    /// let bad_blocks = KBox::pin_init(BadBlocks::new(false), GFP_KERNEL)?;
>> +    /// assert!(!bad_blocks.enabled());
>> +    ///
>> +    /// // Enable it when needed
>> +    /// bad_blocks.enable();
>> +    /// assert!(bad_blocks.enabled());
>> +    ///
>> +    /// // Subsequent enable calls have no effect
>> +    /// bad_blocks.enable();
>> +    /// assert!(bad_blocks.enabled());
>> +    /// # Ok::<(), kernel::error::Error>(())
>> +    /// ```
>> +    pub fn enable(&self) {
>> +        let _ = self.shift_ref().cmpxchg(-1, 0, ordering::Relaxed);
>
> Is there not a C function you can call here? It would be simpler that
> way. Surely drivers don't do this directly.

No, this is the canonical way [1].

[1] https://github.com/torvalds/linux/blob/ba3e43a9e601636f5edb54e259a74f96ca3b8fd8/drivers/block/null_blk/main.c#L563

>
>> +    }
>> +
>> +    /// Checks whether the bad blocks tracker is currently enabled.
>> +    ///
>> +    /// Returns `true` if bad block tracking is active, `false` if it is disabled.
>> +    /// When disabled, the tracker will not perform bad block checks or operations.
>> +    ///
>> +    /// # Returns
>> +    ///
>> +    /// - `true` - Bad block tracking is enabled and operational
>> +    /// - `false` - Bad block tracking is disabled
>
> You explain the meaning of return values twice here. Just drop the
> 'Returns' section.

Ok.

>
>> +    /// # Thread Safety
>> +    ///
>> +    /// This method is thread-safe and uses atomic operations to check the
>> +    /// tracker's state without requiring external synchronization.
>
> This is implicit from the signature of the function.

Will remove.

>
>> +    pub fn set_good(&self, range: impl RangeBounds<u64>) -> Result {
>> +        let range = Self::range(range);
>> +        // SAFETY: By type invariant `self.blocks` is valid. The C function
>> +        // `badblocks_clear` handles synchronization internally.
>> +        unsafe {
>> +            bindings::badblocks_clear(self.blocks.get(), range.start, range.end - range.start)
>> +        }
>> +        .then_some(())
>> +        .ok_or(EINVAL)
>
> then_some() is quite obscure. I would recommend if/else here.

Ok.

Thanks,
Andreas