linux-kernel - Re: [PATCH v2] Revert "sunvdc: Do not spin in an infinite loop when vio_ldc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5b3caa0e218dd473c8871c1b1f09a8dc1c356f1e.camel@physik.fu-berlin.de>
Date: Mon, 06 Oct 2025 15:56:17 +0200
From: John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>
To: Jens Axboe <axboe@...nel.dk>, 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 Mon, 2025-10-06 at 07:46 -0600, Jens Axboe wrote:
> 
> > > The nicer approach would be to have sunvdc punt retries back up to the
> > > block stack, which would at least avoid a busy spin for the condition.
> > > Rather than return BLK_STS_IOERR which terminates the request and
> > > bubbles back the -EIO as per your log. IOW, if we've already spent
> > > 10.5ms in that busy loop as per the above rough calculation, perhaps
> > > we'd be better off restarting the queue and hence this operation after a
> > > small sleeping delay instead. That would seem a lot saner than hammering
> > > on it.
> > 
> > I generally agree with this remark. I just wonder whether this
> > behavior should apply for a logical domain as well. I guess if a
> > request doesn't succeed immediately, it's an urgent problem if the
> > logical domain locks up, is it?
> 
> It's just bad behavior. Honestly most of this just looks like either a
> bad implementation of the protocol as it's all based on busy looping, or
> a badly designed protocol. And then the sunvdc usage of it just
> proliferates that problem, rather than utilizing the tools that exist in
> the block stack to take a breather rather than repeatedly hammering on
> the hardware for conditions like this.

To be fair, the sunvdc driver is fairly old and I'm not sure whether these
tools already existed back then. FWIW, Oracle engineers did actually work
on the Linux for SPARC code for a while and it might be possible that their
UEK kernel tree [1] contains some improvements in this regard.

> > > > And unlike the change in adddc32d6fde ("sunvnet: Do not spin in an infinite
> > > > loop when vio_ldc_send() returns EAGAIN"), we can't just drop data as this
> > > > driver concerns a block device while the other driver concerns a network
> > > > device. Dropping network packages is expected, dropping bytes from a block
> > > > device driver is not.
> > > 
> > > Right, but we can sanely retry it rather than sit in a tight loop.
> > > Something like the entirely untested below diff.
> > > 
> > > diff --git a/drivers/block/sunvdc.c b/drivers/block/sunvdc.c
> > > index db1fe9772a4d..aa49dffb1b53 100644
> > > --- a/drivers/block/sunvdc.c
> > > +++ b/drivers/block/sunvdc.c
> > > @@ -539,6 +539,7 @@ static blk_status_t vdc_queue_rq(struct blk_mq_hw_ctx *hctx,
> > >  	struct vdc_port *port = hctx->queue->queuedata;
> > >  	struct vio_dring_state *dr;
> > >  	unsigned long flags;
> > > +	int ret;
> > >  
> > >  	dr = &port->vio.drings[VIO_DRIVER_TX_RING];
> > >  
> > > @@ -560,7 +561,13 @@ static blk_status_t vdc_queue_rq(struct blk_mq_hw_ctx *hctx,
> > >  		return BLK_STS_DEV_RESOURCE;
> > >  	}
> > >  
> > > -	if (__send_request(bd->rq) < 0) {
> > > +	ret = __send_request(bd->rq);
> > > +	if (ret == -EAGAIN) {
> > > +		spin_unlock_irqrestore(&port->vio.lock, flags);
> > > +		/* already spun for 10msec, defer 10msec and retry */
> > > +		blk_mq_delay_kick_requeue_list(hctx->queue, 10);
> > > +		return BLK_STS_DEV_RESOURCE;
> > > +	} else if (ret < 0) {
> > >  		spin_unlock_irqrestore(&port->vio.lock, flags);
> > >  		return BLK_STS_IOERR;
> > >  	}
> > 
> > We could add this particular change on top of mine after we have
> > extensively tested it.
> 
> It doesn't really make sense on top of yours, as that removes the
> limited looping that sunvdc would do...

Why not? From what I understood, you're moving the limited looping to a
different part of the driver where it can delegate the request back up
the stack meaning that the current place to implement the limitation is
not correct anyway, is it?

> > 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.

Adrian

> [1] https://github.com/oracle/linux-uek/tree/uek4/qu7
> [2] https://github.com/sparclinux/issues/issues

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913