Message-ID: <24f8b266-f17c-4909-b43d-8ab05721c5d8@kernel.dk>
Date: Mon, 6 Oct 2025 08:52:27 -0600
From: Jens Axboe <axboe@...nel.dk>
To: John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>,
 linux-block@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: Andreas Larsson <andreas@...sler.com>,
 Anthony Yznaga <anthony.yznaga@...cle.com>, Sam James <sam@...too.org>,
 "David S . Miller" <davem@...emloft.net>,
 Michael Karcher <kernel@...rcher.dialup.fu-berlin.de>,
 sparclinux@...r.kernel.org
Subject: Re: [PATCH v2] Revert "sunvdc: Do not spin in an infinite loop when
 vio_ldc_send() returns EAGAIN"

On 10/6/25 8:48 AM, John Paul Adrian Glaubitz wrote:
> On Mon, 2025-10-06 at 08:27 -0600, Jens Axboe wrote:
>>>> But that's fine, it's not uncommon for drivers to miss things like that,
>>>> and then we fix them up when noticed. It was probably written by someone
>>>> not super familiar with the IO stack.
>>>
>>> FWIW, Oracle engineers actually made some significant changes to the
>>> driver that they never upstreamed, see:
>>>
>>> https://github.com/oracle/linux-uek/commits/uek4/qu7/drivers/block/sunvdc.c
>>>
>>> In particular, they added support for out-of-order execution:
>>>
>>> https://github.com/oracle/linux-uek/commit/68f7c9c17fb80d29cbc1e5110f6c021f8da8d610
>>>
>>> and they also changed the driver to use the BIO-based interface for
>>> VDC I/O requests:
>>>
>>> https://github.com/oracle/linux-uek/commit/4b725eb64cc10a4877f2af75ff3a776586f68eb7
>>>
>>> Could you review these two changes and tell me whether these would
>>> actually implement the changes you would want to see? I think the BIO
>>> layer is a generic interface of the block layer in the kernel, isn't
>>> it?
>>
>> Moving lower down the stack to use a bio directly is not a good idea,
>> it's in fact going the opposite direction of what we'd like to see in
>> the storage stack. And it would then mean you'd need to implement your
>> own internal requeueing and retrying.
> 
> I looked at the virtio_blk driver and that seems to confirm it. There is no
> use of the bio interface either, so I guess we should not pick up this
> patch.

I'd be very hesitant to pick anything up that hasn't been posted and
included upstream...

> What do you think about the out-of-order execution? Would that make sense
> to upstream it? Does it look reasonable?

I have no opinion on that, there's not even a description of why that
change makes any sense. Sorry but I'm not going to waste my time
reviewing out-of-tree code, it's just not a very useful thing to do. If
the changes get submitted upstream for review in a suitable fashion,
then they will get reviewed.

>> These are the kind of changes that happen when development is done and
>> changes aren't submitted upstream. It's unfortunate drift...
> 
> Well, the problem here is that Oracle stopped working on Linux for SPARC
> abruptly, so many of their improvements were never sent upstream and did
> not see any reviews which would have caught this.

And to be frank, the changes you referenced also look pretty incomplete
and would not pass upstream review. I guess they are dead in the water
at this point, unless someone else picks them up and polishes them into
something that can be sent upstream for review.

>>>>>>> For now, I would propose to pick up my patch to revert the previous
>>>>>>> change. I can then pick up your proposed change and deploy it for
>>>>>>> extensive testing and see if it has any side effects.
>>>>>>
>>>>>> Why not just test this one and see if it works? As far as I can tell,
>>>>>> it's been 6.5 years since this change went in, I can't imagine there's a
>>>>>> huge sense of urgency to fix it up that can't wait for testing a more
>>>>>> proper patch rather than a work-around?
>>>>>
>>>>> Well, the thing is that a lot of people have been running older kernels
>>>>> on SPARC because of issues like these and I have started working on trying
>>>>> to track down all of these issues now [2] for users to be able to run a
>>>>> current kernel. So, the 6.5 years existence of this change shouldn't
>>>>> be an argument I think.
>>>>
>>>> While I agree that the bug is unfortunate, it's also a chance to
>>>> properly fix it rather than just go back to busy looping. How difficult
>>>> is it to test an iteration of the patch? It'd be annoying to queue a
>>>> bandaid only to have to revert that again for a real fix. If this was a
>>>> regression from the last release or two then that'd be a different
>>>> story, but the fact that this has persisted for 6.5 years and is only
>>>> bubbling back up to mainstream now would seem to indicate that we should
>>>> spend a bit of extra time to just get it right the first time.
>>>
>>> We could do that for sure. But I would like to hear your opinion on
>>> the changes contributed by Oracle engineers first. Maybe their
>>> improvements are much better so that it might make sense to try to
>>> upstream them.
>>
>> Won't help this case, and it's actively going the wrong direction
>> imho...
> 
> OK, so your opinion is then to add the patch that you proposed on top
> of what's currently there in Linus' tree, meaning adding some code
> that will requeue requests once the retry limit has been reached?

Right, the patch I sent is against the normal upstream tree.

> Can you maybe post a proper patch then which I (and others) could test
> and then hopefully add their "Tested-by"?

Sure.

-- 
Jens Axboe

Powered by blists - more mailing lists