Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Greg KH]

On Sat, Oct 05, 2024 at 11:48:53AM +0200, Andreas Hindborg wrote:
> Hi Greg,
> 
> "Greg KH" <gregkh@xxxxxxxxxxxxxxxxxxx> writes:
> 
> > On Fri, Oct 04, 2024 at 04:52:24PM +0100, Gary Guo wrote:
> >> There is an operation needed by `block::mq`, atomically decreasing
> >> refcount from 2 to 0, which is not available through refcount.h, so
> >> I exposed `Refcount::as_atomic` which allows accessing the refcount
> >> directly.
> >
> > That's scary, and of course feels wrong on many levels, but:
> >
> >
> >> @@ -91,13 +95,17 @@ pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
> >>      /// C `struct request`. If the operation fails, `this` is returned in the
> >>      /// `Err` variant.
> >>      fn try_set_end(this: ARef<Self>) -> Result<*mut bindings::request, ARef<Self>> {
> >> -        // We can race with `TagSet::tag_to_rq`
> >> -        if let Err(_old) = this.wrapper_ref().refcount().compare_exchange(
> >> -            2,
> >> -            0,
> >> -            Ordering::Relaxed,
> >> -            Ordering::Relaxed,
> >> -        ) {
> >> +        // To hand back the ownership, we need the current refcount to be 2.
> >> +        // Since we can race with `TagSet::tag_to_rq`, this needs to atomically reduce
> >> +        // refcount to 0. `Refcount` does not provide a way to do this, so use the underlying
> >> +        // atomics directly.
> >> +        if this
> >> +            .wrapper_ref()
> >> +            .refcount()
> >> +            .as_atomic()
> >> +            .compare_exchange(2, 0, Ordering::Relaxed, Ordering::Relaxed)
> >> +            .is_err()
> >
> > Why not just call rust_helper_refcount_set()?  Or is the issue that you
> > think you might not be 2 here?  And if you HAVE to be 2, why that magic
> > value (i.e. why not just always be 1 and rely on normal
> > increment/decrement?)
> >
> > I know some refcounts are odd in the kernel, but I don't see where the
> > block layer is caring about 2 as a refcount anywhere, what am I missing?
> 
> It is in the documentation, rendered version available here [1]. Let me
> know if it is still unclear, then I guess we need to update the docs.
> 
> Also, my session from Recipes has a little bit of discussion regarding
> this refcount and it's use [2].

Ah, ick, that's crazy, ok, good luck!