PATCH: fix the barrier IDE detection logic

PATCH: fix the barrier IDE detection logic

[Alan Cox]

This fixes the logic so we always check for the cache. It also defaults
to safer behaviour for the non cache flush case now we have the right bits
in the right places. I've also played a bit with timings - the worst case
timings I can get for the flush are about 7 seconds (which I'd expect
as the engineering worst cases will include retries)

Probably what should happen is that the barrier logic is enabled providing
the wcache is disabled. I've not meddled with this as I don't know what
the intended semantics and rules are for disabling barrier on a live disk
(eg when a user uses hdparm to turn on the write cache). In the current
code as with Jens original that cannot occur.

I've also fixed the new printk's as per a private request from Matt Domsch.

This look right to you Jens ?

--- drivers/ide/ide-disk.c~	2004-08-31 17:35:01.065916488 +0100
+++ drivers/ide/ide-disk.c	2004-08-31 17:41:42.637868272 +0100
@@ -1298,7 +1298,7 @@
 	return 0;
 }
 
-static int write_cache (ide_drive_t *drive, int arg)
+static int write_cache(ide_drive_t *drive, int arg)
 {
 	ide_task_t args;
 	int err;
@@ -1508,7 +1508,7 @@
 		blk_queue_max_sectors(drive->queue, max_s);
 	}
 
-	printk("%s: max request size: %dKiB\n", drive->name, drive->queue->max_sectors / 2);
+	printk(KERN_INFO "%s: max request size: %dKiB\n", drive->name, drive->queue->max_sectors / 2);
 
 	/* Extract geometry if we did not already have one for the drive */
 	if (!drive->cyl || !drive->head || !drive->sect) {
@@ -1537,7 +1537,7 @@
 
 	/* limit drive capacity to 137GB if LBA48 cannot be used */
 	if (drive->addressing == 0 && drive->capacity64 > 1ULL << 28) {
-		printk("%s: cannot use LBA48 - full capacity "
+		printk(KERN_WARNING "%s: cannot use LBA48 - full capacity "
 		       "%llu sectors (%llu MB)\n",
 		       drive->name, (unsigned long long)drive->capacity64,
 		       sectors_to_MB(drive->capacity64));
@@ -1607,23 +1607,31 @@
 	if ((id->csfo & 1) || (id->cfs_enable_1 & (1 << 5)))
 		drive->wcache = 1;
 
-	write_cache(drive, 1);
-
 	/*
-	 * decide if we can sanely support flushes and barriers on
-	 * this drive. unfortunately not all drives advertise FLUSH_CACHE
-	 * support even if they support it. So assume FLUSH_CACHE is there
-	 * always. LBA48 drives are newer, so expect it to flag support
-	 * properly. We can safely support FLUSH_CACHE on lba48, if capacity
-	 * doesn't exceed lba28
+	 * We must avoid issuing commands a drive does not understand
+	 * or we may crash it. We check flush cache is supported. We also
+	 * check we have the LBA48 flush cache if the drive capacity is
+	 * too large. By this time we have trimmed the drive capacity if
+	 * LBA48 is not available so we don't need to recheck that.
 	 */
-	barrier = 1;
+	barrier = 0;
+	if (ide_id_has_flush_cache(id))
+		barrier = 1;
 	if (drive->addressing == 1) {
+		/* Can't issue the correct flush ? */
 		if (capacity > (1ULL << 28) && !ide_id_has_flush_cache_ext(id))
 			barrier = 0;
 	}
+	/* Now we have barrier awareness we can be properly conservative
+	   by default with other drives. We turn off write caching when
+	   barrier is not available. Users can adjust this at runtime if
+	   they need unsafe but fast filesystems. This will reduce the
+	   performance of non cache flush supporting disks but it means
+	   you get the data order guarantees the journalling fs's require */
+	   
+	write_cache(drive, barrier);
 
-	printk("%s: cache flushes %ssupported\n",
+	printk(KERN_DEBUG "%s: cache flushes %ssupported\n",
 		drive->name, barrier ? "" : "not ");
 	if (barrier) {
 		blk_queue_ordered(drive->queue, 1);

Re: PATCH: fix the barrier IDE detection logic

[Bartlomiej Zolnierkiewicz]

On Tuesday 31 August 2004 18:50, Alan Cox wrote:
> This fixes the logic so we always check for the cache. It also defaults
> to safer behaviour for the non cache flush case now we have the right bits
> in the right places. I've also played a bit with timings - the worst case
> timings I can get for the flush are about 7 seconds (which I'd expect
> as the engineering worst cases will include retries)
> 
> Probably what should happen is that the barrier logic is enabled providing
> the wcache is disabled. I've not meddled with this as I don't know what
> the intended semantics and rules are for disabling barrier on a live disk
> (eg when a user uses hdparm to turn on the write cache). In the current
> code as with Jens original that cannot occur.

I think that logic is reversed here, I guess it should be: enable barrier
if user enables wcache and disable it if user disables wcache.

> I've also fixed the new printk's as per a private request from Matt Domsch.

Patch looks fine except:

> +	/* Now we have barrier awareness we can be properly conservative
> +	   by default with other drives. We turn off write caching when
> +	   barrier is not available. Users can adjust this at runtime if

This is not true because there is a check for flush cache in write_cache().

I agree that disabling write cache by default is a good thing but user
should be informed about this fact (ideally there also should be easily
available FAQ somewhere) otherwise we will get a lot of bogus bugreports
about decreased performance...

> +	   they need unsafe but fast filesystems. This will reduce the
> +	   performance of non cache flush supporting disks but it means
> +	   you get the data order guarantees the journalling fs's require */
> +	   
> +	write_cache(drive, barrier);

I'll drop this chunk and resend to Linus.

Thanks!

PS: please also cc: linux-ide on all ATA related stuff

Re: PATCH: fix the barrier IDE detection logic

[Alan Cox]

On Gwe, 2004-09-03 at 14:54, Bartlomiej Zolnierkiewicz wrote:
> I think that logic is reversed here, I guess it should be: enable barrier
> if user enables wcache and disable it if user disables wcache.

Thinking about it from the drive side. If the drive wcache is off then
you have barrier semantics anyway. 

So the logic is something like

has-flush	cache-on		barrier semantics
no-flush	cache-on		random semantics
no-flush	cache-off		barrier semantics
					(the barrier itself is a nop)

> > +	/* Now we have barrier awareness we can be properly conservative
> > +	   by default with other drives. We turn off write caching when
> > +	   barrier is not available. Users can adjust this at runtime if
> 
> This is not true because there is a check for flush cache in write_cache().
> 
> I agree that disabling write cache by default is a good thing but user
> should be informed about this fact (ideally there also should be easily
> available FAQ somewhere) otherwise we will get a lot of bogus bugreports
> about decreased performance...

Agreed. Unfortunately a lot of users are getting burned by journalling
fs's and IDE write caching. As the caches get bigger the risk gets
bigger. SCSI turns it off (and prints a message) so I'd rather see the
write_cache() changed to something like "write_cache_verbose()" and the
printk done than the issue ignored for longer.

Alan

Re: PATCH: fix the barrier IDE detection logic

[Jens Axboe]

On Tue, Aug 31 2004, Alan Cox wrote:
> This fixes the logic so we always check for the cache. It also defaults
> to safer behaviour for the non cache flush case now we have the right bits
> in the right places. I've also played a bit with timings - the worst case
> timings I can get for the flush are about 7 seconds (which I'd expect
> as the engineering worst cases will include retries)

7 seconds isn't too bad, for a write you'll often see larger latencies
in the io scheduler as well.

> Probably what should happen is that the barrier logic is enabled providing
> the wcache is disabled. I've not meddled with this as I don't know what
> the intended semantics and rules are for disabling barrier on a live disk
> (eg when a user uses hdparm to turn on the write cache). In the current
> code as with Jens original that cannot occur.

The code will work just fine if you disable/enable the write cache on
the fly. We'll just return immediately on pre/post flush issues on the
barrier write.

> I've also fixed the new printk's as per a private request from Matt Domsch.
> 
> This look right to you Jens ?

Looks fine, yes thanks.

> 
> --- drivers/ide/ide-disk.c~	2004-08-31 17:35:01.065916488 +0100
> +++ drivers/ide/ide-disk.c	2004-08-31 17:41:42.637868272 +0100
> @@ -1298,7 +1298,7 @@
>  	return 0;
>  }
>  
> -static int write_cache (ide_drive_t *drive, int arg)
> +static int write_cache(ide_drive_t *drive, int arg)
>  {
>  	ide_task_t args;
>  	int err;
> @@ -1508,7 +1508,7 @@
>  		blk_queue_max_sectors(drive->queue, max_s);
>  	}
>  
> -	printk("%s: max request size: %dKiB\n", drive->name, drive->queue->max_sectors / 2);
> +	printk(KERN_INFO "%s: max request size: %dKiB\n", drive->name, drive->queue->max_sectors / 2);
>  
>  	/* Extract geometry if we did not already have one for the drive */
>  	if (!drive->cyl || !drive->head || !drive->sect) {
> @@ -1537,7 +1537,7 @@
>  
>  	/* limit drive capacity to 137GB if LBA48 cannot be used */
>  	if (drive->addressing == 0 && drive->capacity64 > 1ULL << 28) {
> -		printk("%s: cannot use LBA48 - full capacity "
> +		printk(KERN_WARNING "%s: cannot use LBA48 - full capacity "
>  		       "%llu sectors (%llu MB)\n",
>  		       drive->name, (unsigned long long)drive->capacity64,
>  		       sectors_to_MB(drive->capacity64));
> @@ -1607,23 +1607,31 @@
>  	if ((id->csfo & 1) || (id->cfs_enable_1 & (1 << 5)))
>  		drive->wcache = 1;
>  
> -	write_cache(drive, 1);
> -
>  	/*
> -	 * decide if we can sanely support flushes and barriers on
> -	 * this drive. unfortunately not all drives advertise FLUSH_CACHE
> -	 * support even if they support it. So assume FLUSH_CACHE is there
> -	 * always. LBA48 drives are newer, so expect it to flag support
> -	 * properly. We can safely support FLUSH_CACHE on lba48, if capacity
> -	 * doesn't exceed lba28
> +	 * We must avoid issuing commands a drive does not understand
> +	 * or we may crash it. We check flush cache is supported. We also
> +	 * check we have the LBA48 flush cache if the drive capacity is
> +	 * too large. By this time we have trimmed the drive capacity if
> +	 * LBA48 is not available so we don't need to recheck that.
>  	 */
> -	barrier = 1;
> +	barrier = 0;
> +	if (ide_id_has_flush_cache(id))
> +		barrier = 1;
>  	if (drive->addressing == 1) {
> +		/* Can't issue the correct flush ? */
>  		if (capacity > (1ULL << 28) && !ide_id_has_flush_cache_ext(id))
>  			barrier = 0;
>  	}
> +	/* Now we have barrier awareness we can be properly conservative
> +	   by default with other drives. We turn off write caching when
> +	   barrier is not available. Users can adjust this at runtime if
> +	   they need unsafe but fast filesystems. This will reduce the
> +	   performance of non cache flush supporting disks but it means
> +	   you get the data order guarantees the journalling fs's require */
> +	   
> +	write_cache(drive, barrier);
>  
> -	printk("%s: cache flushes %ssupported\n",
> +	printk(KERN_DEBUG "%s: cache flushes %ssupported\n",
>  		drive->name, barrier ? "" : "not ");
>  	if (barrier) {
>  		blk_queue_ordered(drive->queue, 1);
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
Jens Axboe

Re: PATCH: fix the barrier IDE detection logic

[Jens Axboe]

On Fri, Sep 03 2004, Bartlomiej Zolnierkiewicz wrote:
> 
> On Tuesday 31 August 2004 18:50, Alan Cox wrote:
> > This fixes the logic so we always check for the cache. It also defaults
> > to safer behaviour for the non cache flush case now we have the right bits
> > in the right places. I've also played a bit with timings - the worst case
> > timings I can get for the flush are about 7 seconds (which I'd expect
> > as the engineering worst cases will include retries)
> > 
> > Probably what should happen is that the barrier logic is enabled providing
> > the wcache is disabled. I've not meddled with this as I don't know what
> > the intended semantics and rules are for disabling barrier on a live disk
> > (eg when a user uses hdparm to turn on the write cache). In the current
> > code as with Jens original that cannot occur.
> 
> I think that logic is reversed here, I guess it should be: enable barrier
> if user enables wcache and disable it if user disables wcache.

There's no need for changes, ide_queue_flush_cmd() handles this fine
right now.

-- 
Jens Axboe