[PATCH] prevent AoE causing cache aliases

[PATCH] prevent AoE causing cache aliases

[Peter Horton]

This patch prevents the AoE block driver from creating cache aliases of
page cache pages on machines with virtually indexed caches.

Building kernels on an AT91SAM9G20 board without this patch fails with
segmentation faults after a couple of passes.

Signed-off-by: Peter Horton <zero@colonel-panic.org>

Index: linux-2.6.31/drivers/block/aoe/aoecmd.c
===================================================================
--- linux-2.6.31.orig/drivers/block/aoe/aoecmd.c	2009-09-09 23:13:59.000000000 +0100
+++ linux-2.6.31/drivers/block/aoe/aoecmd.c	2009-10-22 10:24:50.000000000 +0100
@@ -735,6 +735,21 @@
 	part_stat_unlock();
 }
 
+/*
+ * Ensure we don't create aliases in VI caches
+ */
+static inline void
+killalias(struct bio *bio)
+{
+	struct bio_vec *bv;
+	int i;
+
+	if (bio_data_dir(bio) == READ)
+		__bio_for_each_segment(bv, bio, i, 0) {
+			flush_dcache_page(bv->bv_page);
+		}
+}
+
 void
 aoecmd_ata_rsp(struct sk_buff *skb)
 {
@@ -853,8 +868,12 @@
 
 	if (buf && --buf->nframesout == 0 && buf->resid == 0) {
 		diskstats(d->gd, buf->bio, jiffies - buf->stime, buf->sector);
-		n = (buf->flags & BUFFL_FAIL) ? -EIO : 0;
-		bio_endio(buf->bio, n);
+		if (buf->flags & BUFFL_FAIL)
+			bio_endio(buf->bio, -EIO);
+		else {
+			killalias(buf->bio);
+			bio_endio(buf->bio, 0);
+		}
 		mempool_free(buf, d->bufpool);
 	}
 

Re: [PATCH] prevent AoE causing cache aliases

[Andrew Morton]

On Thu, 22 Oct 2009 15:22:28 +0100
phorton@bitbox.co.uk (Peter Horton) wrote:

> To: ecashin@coraid.com

Have you heard back from Ed on this?

> Cc: linux-kernel@vger.kernel.org
> Subject: [PATCH] prevent AoE causing cache aliases
> Date: 	Thu, 22 Oct 2009 15:22:28 +0100
> Sender: linux-kernel-owner@vger.kernel.org
> User-Agent: Mutt/1.5.9i
> 
> This patch prevents the AoE block driver from creating cache aliases of
> page cache pages on machines with virtually indexed caches.
> 
> Building kernels on an AT91SAM9G20 board without this patch fails with
> segmentation faults after a couple of passes.
> 
> 
> Index: linux-2.6.31/drivers/block/aoe/aoecmd.c
> ===================================================================
> --- linux-2.6.31.orig/drivers/block/aoe/aoecmd.c	2009-09-09 23:13:59.000000000 +0100
> +++ linux-2.6.31/drivers/block/aoe/aoecmd.c	2009-10-22 10:24:50.000000000 +0100
> @@ -735,6 +735,21 @@
>  	part_stat_unlock();
>  }
>  
> +/*
> + * Ensure we don't create aliases in VI caches
> + */
> +static inline void
> +killalias(struct bio *bio)
> +{
> +	struct bio_vec *bv;
> +	int i;
> +
> +	if (bio_data_dir(bio) == READ)
> +		__bio_for_each_segment(bv, bio, i, 0) {
> +			flush_dcache_page(bv->bv_page);
> +		}
> +}
> +
>  void
>  aoecmd_ata_rsp(struct sk_buff *skb)
>  {
> @@ -853,8 +868,12 @@
>  
>  	if (buf && --buf->nframesout == 0 && buf->resid == 0) {
>  		diskstats(d->gd, buf->bio, jiffies - buf->stime, buf->sector);
> -		n = (buf->flags & BUFFL_FAIL) ? -EIO : 0;
> -		bio_endio(buf->bio, n);
> +		if (buf->flags & BUFFL_FAIL)
> +			bio_endio(buf->bio, -EIO);
> +		else {
> +			killalias(buf->bio);
> +			bio_endio(buf->bio, 0);
> +		}
>  		mempool_free(buf, d->bufpool);
>  	}

Looks OK.

This bugfix will cause a pointless __bio_for_each_segment() busywait
loop to be executed on architectures for which flush_dcache_page() is a
no-op.

We don't have infrastructure to fix that.

Re: [PATCH] prevent AoE causing cache aliases

[Peter Horton]

Andrew Morton wrote:
> On Thu, 22 Oct 2009 15:22:28 +0100
> phorton@bitbox.co.uk (Peter Horton) wrote:
> 
>> To: ecashin@coraid.com
> 
> Have you heard back from Ed on this?
>

No.

>> Cc: linux-kernel@vger.kernel.org
>> Subject: [PATCH] prevent AoE causing cache aliases
>> Date: 	Thu, 22 Oct 2009 15:22:28 +0100
>> Sender: linux-kernel-owner@vger.kernel.org
>> User-Agent: Mutt/1.5.9i
>>
>> This patch prevents the AoE block driver from creating cache aliases of
>> page cache pages on machines with virtually indexed caches.
>>
>> Building kernels on an AT91SAM9G20 board without this patch fails with
>> segmentation faults after a couple of passes.
>>
>>
>> Index: linux-2.6.31/drivers/block/aoe/aoecmd.c
>> ===================================================================
>> --- linux-2.6.31.orig/drivers/block/aoe/aoecmd.c	2009-09-09 23:13:59.000000000 +0100
>> +++ linux-2.6.31/drivers/block/aoe/aoecmd.c	2009-10-22 10:24:50.000000000 +0100
>> @@ -735,6 +735,21 @@
>>  	part_stat_unlock();
>>  }
>>  
>> +/*
>> + * Ensure we don't create aliases in VI caches
>> + */
>> +static inline void
>> +killalias(struct bio *bio)
>> +{
>> +	struct bio_vec *bv;
>> +	int i;
>> +
>> +	if (bio_data_dir(bio) == READ)
>> +		__bio_for_each_segment(bv, bio, i, 0) {
>> +			flush_dcache_page(bv->bv_page);
>> +		}
>> +}
>> +
>>  void
>>  aoecmd_ata_rsp(struct sk_buff *skb)
>>  {
>> @@ -853,8 +868,12 @@
>>  
>>  	if (buf && --buf->nframesout == 0 && buf->resid == 0) {
>>  		diskstats(d->gd, buf->bio, jiffies - buf->stime, buf->sector);
>> -		n = (buf->flags & BUFFL_FAIL) ? -EIO : 0;
>> -		bio_endio(buf->bio, n);
>> +		if (buf->flags & BUFFL_FAIL)
>> +			bio_endio(buf->bio, -EIO);
>> +		else {
>> +			killalias(buf->bio);
>> +			bio_endio(buf->bio, 0);
>> +		}
>>  		mempool_free(buf, d->bufpool);
>>  	}
> 
> Looks OK.
> 
> This bugfix will cause a pointless __bio_for_each_segment() busywait
> loop to be executed on architectures for which flush_dcache_page() is a
> no-op.
> 
> We don't have infrastructure to fix that.

Couldn't we add a flag to the bio that users could set to indicate that 
they are not house trained with respect to the D-cache (i.e non-DMA 
drivers). Architectures that needed to could then flush the relevant 
pages in the bio_endio() path somewhere. At the moment all the non-DMA 
block drivers need to be aware of the cache aliasing issue which means 
this problem keeps arising ...

P.

Re: [PATCH] prevent AoE causing cache aliases

[Andrew Morton]

On Wed, 04 Nov 2009 10:54:34 +0000 Peter Horton <phorton@bitbox.co.uk> wrote:

> Andrew Morton wrote:
> > On Thu, 22 Oct 2009 15:22:28 +0100
> > phorton@bitbox.co.uk (Peter Horton) wrote:
> > 
> >> To: ecashin@coraid.com
> > 
> > Have you heard back from Ed on this?
> >
> 
> No.
> 
> >> Cc: linux-kernel@vger.kernel.org
> >> Subject: [PATCH] prevent AoE causing cache aliases
> >> Date: 	Thu, 22 Oct 2009 15:22:28 +0100
> >> Sender: linux-kernel-owner@vger.kernel.org
> >> User-Agent: Mutt/1.5.9i
> >>
> >> This patch prevents the AoE block driver from creating cache aliases of
> >> page cache pages on machines with virtually indexed caches.
> >>
> >> Building kernels on an AT91SAM9G20 board without this patch fails with
> >> segmentation faults after a couple of passes.
> >>
> >>
> >> Index: linux-2.6.31/drivers/block/aoe/aoecmd.c
> >> ===================================================================
> >> --- linux-2.6.31.orig/drivers/block/aoe/aoecmd.c	2009-09-09 23:13:59.000000000 +0100
> >> +++ linux-2.6.31/drivers/block/aoe/aoecmd.c	2009-10-22 10:24:50.000000000 +0100
> >> @@ -735,6 +735,21 @@
> >>  	part_stat_unlock();
> >>  }
> >>  
> >> +/*
> >> + * Ensure we don't create aliases in VI caches
> >> + */
> >> +static inline void
> >> +killalias(struct bio *bio)
> >> +{
> >> +	struct bio_vec *bv;
> >> +	int i;
> >> +
> >> +	if (bio_data_dir(bio) == READ)
> >> +		__bio_for_each_segment(bv, bio, i, 0) {
> >> +			flush_dcache_page(bv->bv_page);
> >> +		}
> >> +}
> >> +
> >>  void
> >>  aoecmd_ata_rsp(struct sk_buff *skb)
> >>  {
> >> @@ -853,8 +868,12 @@
> >>  
> >>  	if (buf && --buf->nframesout == 0 && buf->resid == 0) {
> >>  		diskstats(d->gd, buf->bio, jiffies - buf->stime, buf->sector);
> >> -		n = (buf->flags & BUFFL_FAIL) ? -EIO : 0;
> >> -		bio_endio(buf->bio, n);
> >> +		if (buf->flags & BUFFL_FAIL)
> >> +			bio_endio(buf->bio, -EIO);
> >> +		else {
> >> +			killalias(buf->bio);
> >> +			bio_endio(buf->bio, 0);
> >> +		}
> >>  		mempool_free(buf, d->bufpool);
> >>  	}
> > 
> > Looks OK.
> > 
> > This bugfix will cause a pointless __bio_for_each_segment() busywait
> > loop to be executed on architectures for which flush_dcache_page() is a
> > no-op.
> > 
> > We don't have infrastructure to fix that.
> 
> Couldn't we add a flag to the bio that users could set to indicate that 
> they are not house trained with respect to the D-cache (i.e non-DMA 
> drivers). Architectures that needed to could then flush the relevant 
> pages in the bio_endio() path somewhere. At the moment all the non-DMA 
> block drivers need to be aware of the cache aliasing issue which means 
> this problem keeps arising ...
> 

Could.  We'll need to change each arch _somehow_.  Even if it's a
matter of adding `#define i_am_not_house_trained' to the troublesome
ones or something, then ifdeffing existing code.

I was thinking that a general bio_flush_dcache_pages() in block core
(or in each arch) would be a suitable way to handle this but I was
unable to find other drivers which needed it after a brief search.

Re: [PATCH] prevent AoE causing cache aliases

[Peter Horton]

Andrew Morton wrote:
> On Wed, 04 Nov 2009 10:54:34 +0000 Peter Horton <phorton@bitbox.co.uk> wrote:
> 
>> Andrew Morton wrote:
>>> On Thu, 22 Oct 2009 15:22:28 +0100
>>> phorton@bitbox.co.uk (Peter Horton) wrote:
>>>
>>>> To: ecashin@coraid.com
>>> Have you heard back from Ed on this?
>>>
>> No.
>>
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Subject: [PATCH] prevent AoE causing cache aliases
>>>> Date: 	Thu, 22 Oct 2009 15:22:28 +0100
>>>> Sender: linux-kernel-owner@vger.kernel.org
>>>> User-Agent: Mutt/1.5.9i
>>>>
>>>> This patch prevents the AoE block driver from creating cache aliases of
>>>> page cache pages on machines with virtually indexed caches.
>>>>
>>>> Building kernels on an AT91SAM9G20 board without this patch fails with
>>>> segmentation faults after a couple of passes.
>>>>
>>>>
>>>> Index: linux-2.6.31/drivers/block/aoe/aoecmd.c
>>>> ===================================================================
>>>> --- linux-2.6.31.orig/drivers/block/aoe/aoecmd.c	2009-09-09 23:13:59.000000000 +0100
>>>> +++ linux-2.6.31/drivers/block/aoe/aoecmd.c	2009-10-22 10:24:50.000000000 +0100
>>>> @@ -735,6 +735,21 @@
>>>>  	part_stat_unlock();
>>>>  }
>>>>  
>>>> +/*
>>>> + * Ensure we don't create aliases in VI caches
>>>> + */
>>>> +static inline void
>>>> +killalias(struct bio *bio)
>>>> +{
>>>> +	struct bio_vec *bv;
>>>> +	int i;
>>>> +
>>>> +	if (bio_data_dir(bio) == READ)
>>>> +		__bio_for_each_segment(bv, bio, i, 0) {
>>>> +			flush_dcache_page(bv->bv_page);
>>>> +		}
>>>> +}
>>>> +
>>>>  void
>>>>  aoecmd_ata_rsp(struct sk_buff *skb)
>>>>  {
>>>> @@ -853,8 +868,12 @@
>>>>  
>>>>  	if (buf && --buf->nframesout == 0 && buf->resid == 0) {
>>>>  		diskstats(d->gd, buf->bio, jiffies - buf->stime, buf->sector);
>>>> -		n = (buf->flags & BUFFL_FAIL) ? -EIO : 0;
>>>> -		bio_endio(buf->bio, n);
>>>> +		if (buf->flags & BUFFL_FAIL)
>>>> +			bio_endio(buf->bio, -EIO);
>>>> +		else {
>>>> +			killalias(buf->bio);
>>>> +			bio_endio(buf->bio, 0);
>>>> +		}
>>>>  		mempool_free(buf, d->bufpool);
>>>>  	}
>>> Looks OK.
>>>
>>> This bugfix will cause a pointless __bio_for_each_segment() busywait
>>> loop to be executed on architectures for which flush_dcache_page() is a
>>> no-op.
>>>
>>> We don't have infrastructure to fix that.
>> Couldn't we add a flag to the bio that users could set to indicate that 
>> they are not house trained with respect to the D-cache (i.e non-DMA 
>> drivers). Architectures that needed to could then flush the relevant 
>> pages in the bio_endio() path somewhere. At the moment all the non-DMA 
>> block drivers need to be aware of the cache aliasing issue which means 
>> this problem keeps arising ...
>>
> 
> Could.  We'll need to change each arch _somehow_.  Even if it's a
> matter of adding `#define i_am_not_house_trained' to the troublesome
> ones or something, then ifdeffing existing code.
> 
> I was thinking that a general bio_flush_dcache_pages() in block core
> (or in each arch) would be a suitable way to handle this but I was
> unable to find other drivers which needed it after a brief search.
> 
> 

IDE does it at a lower level (arch/mips/include/asm/mach-generic/ide.h 
for example).

Looks like the generic PIO ops in drivers/ata/libata-sff.c could cause 
problems too.

I think the problem is often masked by the small cache sizes on the 
platforms with VI caches. The original AoE problem I see is on an ARM926 
with 32K D-cache, I don't see the problem at all on another ARM926 with 
16K D-cache.

P.

Re: [PATCH] prevent AoE causing cache aliases

[Jens Axboe]

On Wed, Nov 04 2009, Andrew Morton wrote:
> On Wed, 04 Nov 2009 10:54:34 +0000 Peter Horton <phorton@bitbox.co.uk> wrote:
> 
> > Andrew Morton wrote:
> > > On Thu, 22 Oct 2009 15:22:28 +0100
> > > phorton@bitbox.co.uk (Peter Horton) wrote:
> > > 
> > >> To: ecashin@coraid.com
> > > 
> > > Have you heard back from Ed on this?
> > >
> > 
> > No.
> > 
> > >> Cc: linux-kernel@vger.kernel.org
> > >> Subject: [PATCH] prevent AoE causing cache aliases
> > >> Date: 	Thu, 22 Oct 2009 15:22:28 +0100
> > >> Sender: linux-kernel-owner@vger.kernel.org
> > >> User-Agent: Mutt/1.5.9i
> > >>
> > >> This patch prevents the AoE block driver from creating cache aliases of
> > >> page cache pages on machines with virtually indexed caches.
> > >>
> > >> Building kernels on an AT91SAM9G20 board without this patch fails with
> > >> segmentation faults after a couple of passes.
> > >>
> > >>
> > >> Index: linux-2.6.31/drivers/block/aoe/aoecmd.c
> > >> ===================================================================
> > >> --- linux-2.6.31.orig/drivers/block/aoe/aoecmd.c	2009-09-09 23:13:59.000000000 +0100
> > >> +++ linux-2.6.31/drivers/block/aoe/aoecmd.c	2009-10-22 10:24:50.000000000 +0100
> > >> @@ -735,6 +735,21 @@
> > >>  	part_stat_unlock();
> > >>  }
> > >>  
> > >> +/*
> > >> + * Ensure we don't create aliases in VI caches
> > >> + */
> > >> +static inline void
> > >> +killalias(struct bio *bio)
> > >> +{
> > >> +	struct bio_vec *bv;
> > >> +	int i;
> > >> +
> > >> +	if (bio_data_dir(bio) == READ)
> > >> +		__bio_for_each_segment(bv, bio, i, 0) {
> > >> +			flush_dcache_page(bv->bv_page);
> > >> +		}
> > >> +}
> > >> +
> > >>  void
> > >>  aoecmd_ata_rsp(struct sk_buff *skb)
> > >>  {
> > >> @@ -853,8 +868,12 @@
> > >>  
> > >>  	if (buf && --buf->nframesout == 0 && buf->resid == 0) {
> > >>  		diskstats(d->gd, buf->bio, jiffies - buf->stime, buf->sector);
> > >> -		n = (buf->flags & BUFFL_FAIL) ? -EIO : 0;
> > >> -		bio_endio(buf->bio, n);
> > >> +		if (buf->flags & BUFFL_FAIL)
> > >> +			bio_endio(buf->bio, -EIO);
> > >> +		else {
> > >> +			killalias(buf->bio);
> > >> +			bio_endio(buf->bio, 0);
> > >> +		}
> > >>  		mempool_free(buf, d->bufpool);
> > >>  	}
> > > 
> > > Looks OK.
> > > 
> > > This bugfix will cause a pointless __bio_for_each_segment() busywait
> > > loop to be executed on architectures for which flush_dcache_page() is a
> > > no-op.
> > > 
> > > We don't have infrastructure to fix that.
> > 
> > Couldn't we add a flag to the bio that users could set to indicate that 
> > they are not house trained with respect to the D-cache (i.e non-DMA 
> > drivers). Architectures that needed to could then flush the relevant 
> > pages in the bio_endio() path somewhere. At the moment all the non-DMA 
> > block drivers need to be aware of the cache aliasing issue which means 
> > this problem keeps arising ...
> > 
> 
> Could.  We'll need to change each arch _somehow_.  Even if it's a
> matter of adding `#define i_am_not_house_trained' to the troublesome
> ones or something, then ifdeffing existing code.
> 
> I was thinking that a general bio_flush_dcache_pages() in block core
> (or in each arch) would be a suitable way to handle this but I was
> unable to find other drivers which needed it after a brief search.

Indeed, we should have such a helper. I can't find any ARCH define that
tells us when we need to do this. Easiest is probably to grep in arch/
for non-empty definitions of flush_dcache_page() and add such a define.

I'll hack one up.

-- 
Jens Axboe

Re: [PATCH] prevent AoE causing cache aliases

[Ed Cashin]

On Tue, Nov 03, 2009 at 04:37:55PM -0800, Andrew Morton wrote:
> On Thu, 22 Oct 2009 15:22:28 +0100
> phorton@bitbox.co.uk (Peter Horton) wrote:
> 
> > To: ecashin@coraid.com
> 
> Have you heard back from Ed on this?

Sorry, I didn't comment because I don't have much experience
with virtually indexed caches, and while the fix seems to
make sense, I was hoping to learn more from the discussion
to follow.

I do think that it would be helpful to spell out "virtually
indexed" in the comment.  "VI" might not be enough to jog
the memory of folks who haven't used such an architecture
recently.

-- 
  Ed