[PATCH] ata: fix sleep-while-holding-spinlock in sata_nv

[PATCH] ata: fix sleep-while-holding-spinlock in sata_nv

[Arjan van de Ven]

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH] ata: fix sleep-while-holding-spinlock in sata_nv

blk_queue_bounce_limit() is a sleeping function, so reorganize
the code a little to not call it while holding a spinlock.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 drivers/ata/sata_nv.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index 858f706..a7bc56d 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -671,6 +671,11 @@ static int nv_adma_slave_config(struct scsi_device *sdev)
 	port1 = ap->host->ports[1]->private_data;
 	sdev0 = ap->host->ports[0]->link.device[0].sdev;
 	sdev1 = ap->host->ports[1]->link.device[0].sdev;
+
+	blk_queue_segment_boundary(sdev->request_queue, segment_boundary);
+	blk_queue_max_hw_segments(sdev->request_queue, sg_tablesize);
+	spin_unlock_irqrestore(ap->lock, flags);
+
 	if ((port0->flags & NV_ADMA_ATAPI_SETUP_COMPLETE) ||
 	    (port1->flags & NV_ADMA_ATAPI_SETUP_COMPLETE)) {
 		/** We have to set the DMA mask to 32-bit if either port is in
@@ -701,14 +706,11 @@ static int nv_adma_slave_config(struct scsi_device *sdev)
 					       pp->adma_dma_mask);
 	}
 
-	blk_queue_segment_boundary(sdev->request_queue, segment_boundary);
-	blk_queue_max_hw_segments(sdev->request_queue, sg_tablesize);
 	ata_port_printk(ap, KERN_INFO,
 		"DMA mask 0x%llX, segment boundary 0x%lX, hw segs %hu\n",
 		(unsigned long long)*ap->host->dev->dma_mask,
 		segment_boundary, sg_tablesize);
 
-	spin_unlock_irqrestore(ap->lock, flags);
 
 	return rc;
 }
-- 
1.5.4.5

Re: [PATCH] ata: fix sleep-while-holding-spinlock in sata_nv

[Robert Hancock]

Arjan van de Ven wrote:
> From: Arjan van de Ven <arjan@linux.intel.com>
> Subject: [PATCH] ata: fix sleep-while-holding-spinlock in sata_nv
> 
> blk_queue_bounce_limit() is a sleeping function, so reorganize
> the code a little to not call it while holding a spinlock.
> 
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> ---
>  drivers/ata/sata_nv.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
> index 858f706..a7bc56d 100644
> --- a/drivers/ata/sata_nv.c
> +++ b/drivers/ata/sata_nv.c
> @@ -671,6 +671,11 @@ static int nv_adma_slave_config(struct scsi_device *sdev)
>  	port1 = ap->host->ports[1]->private_data;
>  	sdev0 = ap->host->ports[0]->link.device[0].sdev;
>  	sdev1 = ap->host->ports[1]->link.device[0].sdev;
> +
> +	blk_queue_segment_boundary(sdev->request_queue, segment_boundary);
> +	blk_queue_max_hw_segments(sdev->request_queue, sg_tablesize);
> +	spin_unlock_irqrestore(ap->lock, flags);
> +
>  	if ((port0->flags & NV_ADMA_ATAPI_SETUP_COMPLETE) ||
>  	    (port1->flags & NV_ADMA_ATAPI_SETUP_COMPLETE)) {
>  		/** We have to set the DMA mask to 32-bit if either port is in
> @@ -701,14 +706,11 @@ static int nv_adma_slave_config(struct scsi_device *sdev)
>  					       pp->adma_dma_mask);
>  	}
>  
> -	blk_queue_segment_boundary(sdev->request_queue, segment_boundary);
> -	blk_queue_max_hw_segments(sdev->request_queue, sg_tablesize);
>  	ata_port_printk(ap, KERN_INFO,
>  		"DMA mask 0x%llX, segment boundary 0x%lX, hw segs %hu\n",
>  		(unsigned long long)*ap->host->dev->dma_mask,
>  		segment_boundary, sg_tablesize);
>  
> -	spin_unlock_irqrestore(ap->lock, flags);
>  
>  	return rc;
>  }

I'm not certain this is safe to do it quite this way. It would be better 
to keep that spinlock held so that no operations could be in progress on 
either port while these operations are happening.

It would be better to fix the regression from 
419c434c35614609fd0c79d335c134bf4b88b30b in block/blk_settings.c that 
resulted in the blk_queue_bounce_limit allocation wrongly allocating 
emergency ISA pages in the first place as a 32-bit DMA mask does not 
need them.

Re: [PATCH] ata: fix sleep-while-holding-spinlock in sata_nv

[Arjan van de Ven]

On Tue, 20 May 2008 20:17:08 -0600
Robert Hancock <hancockr@shaw.ca> wrote:

> Arjan van de Ven wrote:
> > From: Arjan van de Ven <arjan@linux.intel.com>
> > Subject: [PATCH] ata: fix sleep-while-holding-spinlock in sata_nv
> > 
> > blk_queue_bounce_limit() is a sleeping function, so reorganize
> > the code a little to not call it while holding a spinlock.
> > 
> > Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> > ---
> >  drivers/ata/sata_nv.c |    8 +++++---
> >  1 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
> > index 858f706..a7bc56d 100644
> > --- a/drivers/ata/sata_nv.c
> > +++ b/drivers/ata/sata_nv.c
> > @@ -671,6 +671,11 @@ static int nv_adma_slave_config(struct
> > scsi_device *sdev) port1 = ap->host->ports[1]->private_data;
> >  	sdev0 = ap->host->ports[0]->link.device[0].sdev;
> >  	sdev1 = ap->host->ports[1]->link.device[0].sdev;
> > +
> > +	blk_queue_segment_boundary(sdev->request_queue,
> > segment_boundary);
> > +	blk_queue_max_hw_segments(sdev->request_queue,
> > sg_tablesize);
> > +	spin_unlock_irqrestore(ap->lock, flags);
> > +
> >  	if ((port0->flags & NV_ADMA_ATAPI_SETUP_COMPLETE) ||
> >  	    (port1->flags & NV_ADMA_ATAPI_SETUP_COMPLETE)) {
> >  		/** We have to set the DMA mask to 32-bit if
> > either port is in @@ -701,14 +706,11 @@ static int
> > nv_adma_slave_config(struct scsi_device *sdev) pp->adma_dma_mask);
> >  	}
> >  
> > -	blk_queue_segment_boundary(sdev->request_queue,
> > segment_boundary);
> > -	blk_queue_max_hw_segments(sdev->request_queue,
> > sg_tablesize); ata_port_printk(ap, KERN_INFO,
> >  		"DMA mask 0x%llX, segment boundary 0x%lX, hw segs
> > %hu\n", (unsigned long long)*ap->host->dev->dma_mask,
> >  		segment_boundary, sg_tablesize);
> >  
> > -	spin_unlock_irqrestore(ap->lock, flags);
> >  
> >  	return rc;
> >  }
> 
> I'm not certain this is safe to do it quite this way. It would be
> better to keep that spinlock held so that no operations could be in
> progress on either port while these operations are happening.

blk_bounce_limit can sleep. that's just a fact of life ;(

> 
> It would be better to fix the regression from 
> 419c434c35614609fd0c79d335c134bf4b88b30b in block/blk_settings.c that 
> resulted in the blk_queue_bounce_limit allocation wrongly allocating 
> emergency ISA pages in the first place as a 32-bit DMA mask does not 
> need them.

the condition under which it sleeps might be slightly buggy on your
exact x86 machine... but that doesn't mean that that is guaranteed to
be so forever going forward.... it's still a sleeping function.

Re: [PATCH] ata: fix sleep-while-holding-spinlock in sata_nv

[Robert Hancock]

rjan van de Ven wrote:
>> I'm not certain this is safe to do it quite this way. It would be
>> better to keep that spinlock held so that no operations could be in
>> progress on either port while these operations are happening.
> 
> blk_bounce_limit can sleep. that's just a fact of life ;(

Now it can, for no reason. Under the conditions it was used before, it 
never could.

> 
>> It would be better to fix the regression from 
>> 419c434c35614609fd0c79d335c134bf4b88b30b in block/blk_settings.c that 
>> resulted in the blk_queue_bounce_limit allocation wrongly allocating 
>> emergency ISA pages in the first place as a 32-bit DMA mask does not 
>> need them.
> 
> the condition under which it sleeps might be slightly buggy on your
> exact x86 machine... but that doesn't mean that that is guaranteed to
> be so forever going forward.... it's still a sleeping function.

More than slightly buggy, I think.. It seems like it is going to be 
bouncing block layer accesses to devices with 32-bit DMA masks through 
the 16MB ZONE_DMA. If that's what's actually going on, I'm surprised 
there haven't been more regression reports. The fact that the function 
now sleeps when it didn't before is the least of the problems here..

Re: [PATCH] ata: fix sleep-while-holding-spinlock in sata_nv

[Arjan van de Ven]

On Wed, 21 May 2008 08:31:26 -0600
Robert Hancock <hancockr@shaw.ca> wrote:

> rjan van de Ven wrote:
> >> I'm not certain this is safe to do it quite this way. It would be
> >> better to keep that spinlock held so that no operations could be in
> >> progress on either port while these operations are happening.
> > 
> > blk_bounce_limit can sleep. that's just a fact of life ;(
> 
> Now it can, for no reason. Under the conditions it was used before,
> it never could.

"never" is.. a dangerous assumption. It depends on the details of what
GFP_DMA exactly means and how the exact zoning in the system is set up.
All of that is undergoing flux (Andi Kleen has been hacking on that for
example)

But yes, on the existing VM layout on a 32 bit X86 system it wouldn't
sleep if you set the mask to exactly 32 bit (except for the bug)

> > the condition under which it sleeps might be slightly buggy on your
> > exact x86 machine... but that doesn't mean that that is guaranteed
> > to be so forever going forward.... it's still a sleeping function.
> 
> More than slightly buggy, I think.. It seems like it is going to be 
> bouncing block layer accesses to devices with 32-bit DMA masks
> through the 16MB ZONE_DMA. If that's what's actually going on, I'm
> surprised there haven't been more regression reports. The fact that
> the function now sleeps when it didn't before is the least of the
> problems here..

you're absolutely right that the current implementation has a bug.
But the sleepy-ness part isn't really part of that bug. When we get
pools to fill for other masks we'll need to sleep as well. Heck,
it's not unthinkable that swiotlb on 64 bit will go do the same...