[PATCH] mmc: Multi-sector writes

[PATCH] mmc: Multi-sector writes

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 439 bytes --]

Adds support for writing multiple sectors at once. This allows
back-to-back transfers of sectors giving roughly double write throughput.

To be able to detect which sector is causing problems the system falls
back to single sector writes if a failure is detected.

Tested by several people with no side-effects found.

Signed-off-by: Pierre Ossman <drzeus@drzeus.cx>

---

Previously submitted: 2005-03-16
Previously submitted: 2004-12-03

[-- Attachment #2: mmc-bulk.patch --]
[-- Type: text/x-patch, Size: 3619 bytes --]

Index: linux-wbsd/drivers/mmc/mmc_block.c
===================================================================
--- linux-wbsd/drivers/mmc/mmc_block.c	(revision 77)
+++ linux-wbsd/drivers/mmc/mmc_block.c	(working copy)
@@ -166,9 +166,25 @@
 	struct mmc_blk_data *md = mq->data;
 	struct mmc_card *card = md->queue.card;
 	int ret;
+	
+#ifdef CONFIG_MMC_BULKTRANSFER
+	int failsafe;
+#endif
 
 	if (mmc_card_claim_host(card))
 		goto cmd_err;
+	
+#ifdef CONFIG_MMC_BULKTRANSFER
+	/*
+	 * We first try transfering multiple blocks. If this fails
+	 * we fall back to single block transfers.
+	 *
+	 * This gives us good performance when all is well and the
+	 * possibility to determine which sector fails when all
+	 * is not well.
+	 */
+	failsafe = 0;
+#endif
 
 	do {
 		struct mmc_blk_request brq;
@@ -189,14 +205,32 @@
 		brq.stop.arg = 0;
 		brq.stop.flags = MMC_RSP_R1B;
 
+#ifdef CONFIG_MMC_BULKTRANSFER		
+		/*
+		 * A multi-block transfer failed. Falling back to single
+		 * blocks.
+		 */
+		if (failsafe)
+			brq.data.blocks = 1;
+		
+#else
+		/*
+		 * Writes are done one sector at a time.
+		 */
+		if (rq_data_dir(req) != READ)
+			brq.data.blocks = 1;
+#endif
+		
+		ret = 1;
+
 		if (rq_data_dir(req) == READ) {
 			brq.cmd.opcode = brq.data.blocks > 1 ? MMC_READ_MULTIPLE_BLOCK : MMC_READ_SINGLE_BLOCK;
 			brq.data.flags |= MMC_DATA_READ;
 		} else {
-			brq.cmd.opcode = MMC_WRITE_BLOCK;
+			brq.cmd.opcode = brq.data.blocks > 1 ? MMC_WRITE_MULTIPLE_BLOCK :
+				MMC_WRITE_BLOCK;
 			brq.cmd.flags = MMC_RSP_R1B;
 			brq.data.flags |= MMC_DATA_WRITE;
-			brq.data.blocks = 1;
 		}
 		brq.mrq.stop = brq.data.blocks > 1 ? &brq.stop : NULL;
 
@@ -204,19 +238,19 @@
 		if (brq.cmd.error) {
 			printk(KERN_ERR "%s: error %d sending read/write command\n",
 			       req->rq_disk->disk_name, brq.cmd.error);
-			goto cmd_err;
+			goto cmd_fail;
 		}
 
 		if (brq.data.error) {
 			printk(KERN_ERR "%s: error %d transferring data\n",
 			       req->rq_disk->disk_name, brq.data.error);
-			goto cmd_err;
+			goto cmd_fail;
 		}
 
 		if (brq.stop.error) {
 			printk(KERN_ERR "%s: error %d sending stop command\n",
 			       req->rq_disk->disk_name, brq.stop.error);
-			goto cmd_err;
+			goto cmd_fail;
 		}
 
 		do {
@@ -229,7 +263,7 @@
 			if (err) {
 				printk(KERN_ERR "%s: error %d requesting status\n",
 				       req->rq_disk->disk_name, err);
-				goto cmd_err;
+				goto cmd_fail;
 			}
 		} while (!(cmd.resp[0] & R1_READY_FOR_DATA));
 
@@ -255,6 +289,27 @@
 			end_that_request_last(req);
 		}
 		spin_unlock_irq(&md->lock);
+		
+#ifdef CONFIG_MMC_BULKTRANSFER
+		/*
+		 * Go back to bulk mode if in failsafe mode.
+		 */
+		failsafe = 0;
+#endif
+
+		continue;
+
+ cmd_fail:
+
+#ifdef CONFIG_MMC_BULKTRANSFER
+		if (failsafe)
+	 		goto cmd_err;
+	 	else
+	 		failsafe = 1;
+#else
+ 		goto cmd_err;
+#endif
+
 	} while (ret);
 
 	mmc_card_release_host(card);
Index: linux-wbsd/drivers/mmc/Kconfig
===================================================================
--- linux-wbsd/drivers/mmc/Kconfig	(revision 96)
+++ linux-wbsd/drivers/mmc/Kconfig	(working copy)
@@ -41,6 +41,15 @@
 	  mount the filesystem. Almost everyone wishing MMC support
 	  should say Y or M here.
 
+config MMC_BULKTRANSFER
+	bool "Multi-block writes (EXPERIMENTAL)"
+	depends on MMC_BLOCK != n && EXPERIMENTAL
+	default n
+	help
+	  By default all writes are done one sector at a time. Enable
+	  this option to transfer as large blocks as the host supports.
+	  The transfer speed is in most cases doubled.
+
 config MMC_ARMMMCI
 	tristate "ARM AMBA Multimedia Card Interface support"
 	depends on ARM_AMBA && MMC

Re: [PATCH] mmc: Multi-sector writes

[Andrew Morton]

Pierre Ossman <drzeus-list@drzeus.cx> wrote:
>
> Adds support for writing multiple sectors at once. This allows
> back-to-back transfers of sectors giving roughly double write throughput.
> 
> To be able to detect which sector is causing problems the system falls
> back to single sector writes if a failure is detected.
> 
> ...
> --- linux-wbsd/drivers/mmc/mmc_block.c	(revision 77)
> +++ linux-wbsd/drivers/mmc/mmc_block.c	(working copy)
> @@ -166,9 +166,25 @@
>  	struct mmc_blk_data *md = mq->data;
>  	struct mmc_card *card = md->queue.card;
>  	int ret;
> +	
> +#ifdef CONFIG_MMC_BULKTRANSFER
> +	int failsafe;
> +#endif
>  
>  	if (mmc_card_claim_host(card))
>  		goto cmd_err;
> +	
> +#ifdef CONFIG_MMC_BULKTRANSFER
> +	/*
> +	 * We first try transfering multiple blocks. If this fails
> +	 * we fall back to single block transfers.
> +	 *
> +	 * This gives us good performance when all is well and the
> +	 * possibility to determine which sector fails when all
> +	 * is not well.
> +	 */
> +	failsafe = 0;
> +#endif
> 

The fact that this is enabled under the experimental
CONFIG_MMC_BULKTRANSFER seems unfortunate.  I mean, if the code works OK
then we should just enable it unconditionally, no?

I'm thinking that it would be better to not have the config option there
and then re-add it late in the 2.6.14 cycle if someone reports problems
which cannot be fixed.  Or at least make it default to 'y' so we get more
testing coverage, then remove the config option later.  Or something.

Thoughts?

Re: [PATCH] mmc: Multi-sector writes

[Pierre Ossman]

Andrew Morton wrote:

>The fact that this is enabled under the experimental
>CONFIG_MMC_BULKTRANSFER seems unfortunate.  I mean, if the code works OK
>then we should just enable it unconditionally, no?
>
>  
>

It was made this way to make Russell more open to it. I have since not
recieved any more comments from him so I figured I could pass it by you
instead to get more wide spread testing. The long term goal was removing
the config and having it on all the time.

>I'm thinking that it would be better to not have the config option there
>and then re-add it late in the 2.6.14 cycle if someone reports problems
>which cannot be fixed.  Or at least make it default to 'y' so we get more
>testing coverage, then remove the config option later.  Or something.
>
>Thoughts?
>  
>

Removing it would be preferable by me. All that #ifdef tends to clutter
up the code. After som initial problem with a buggy card everything has
worked flawlesly.

Rgds
Pierre

Re: [PATCH] mmc: Multi-sector writes

[Andrew Morton]

Pierre Ossman <drzeus-list@drzeus.cx> wrote:
>
> >I'm thinking that it would be better to not have the config option there
>  >and then re-add it late in the 2.6.14 cycle if someone reports problems
>  >which cannot be fixed.  Or at least make it default to 'y' so we get more
>  >testing coverage, then remove the config option later.  Or something.
>  >
>  >Thoughts?
>  >  
>  >
> 
>  Removing it would be preferable by me. All that #ifdef tends to clutter
>  up the code. After som initial problem with a buggy card everything has
>  worked flawlesly.

OK..  Please send an additional patch for that sometime?

Re: [PATCH] mmc: Multi-sector writes

[Russell King]

On Wed, Aug 17, 2005 at 10:48:05PM -0700, Andrew Morton wrote:
> Pierre Ossman <drzeus-list@drzeus.cx> wrote:
> >
> > >I'm thinking that it would be better to not have the config option there
> >  >and then re-add it late in the 2.6.14 cycle if someone reports problems
> >  >which cannot be fixed.  Or at least make it default to 'y' so we get more
> >  >testing coverage, then remove the config option later.  Or something.
> >  >
> >  >Thoughts?
> >  >  
> >  >
> > 
> >  Removing it would be preferable by me. All that #ifdef tends to clutter
> >  up the code. After som initial problem with a buggy card everything has
> >  worked flawlesly.
> 
> OK..  Please send an additional patch for that sometime?

I'd rather not.  The problem is that we have a host (thanks Intel)
which is unable to report how many bytes were transferred before an
error occurs.  My fear is that doing anything other than sector by
sector write will lead to corruption should an error occur.

However, I've no way to induce such an error, so I can only base
this on theory.

It may work perfectly for the case when everything's operating
correctly, but I suspect if you're going to do multi-sector writes,
it'll all fall apart on the first error, especially on this host.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] mmc: Multi-sector writes

[Pierre Ossman]

Russell King wrote:

>
>I'd rather not.  The problem is that we have a host (thanks Intel)
>which is unable to report how many bytes were transferred before an
>error occurs.  My fear is that doing anything other than sector by
>sector write will lead to corruption should an error occur.
>
>However, I've no way to induce such an error, so I can only base
>this on theory.
>
>It may work perfectly for the case when everything's operating
>correctly, but I suspect if you're going to do multi-sector writes,
>it'll all fall apart on the first error, especially on this host.
>
>  
>

We had this discussion on LKML and Alan Cox' comment on it was that a
solution like this would be acceptable, where we try and shove
everything out first and then fall back on sector-by-sector to determine
where an error occurs. This will only break if the problematic sector
keeps shifting around, but at that point the card is probably toast
anyway (if the thing keeps moving how can you bad block it?).

Rgds
Pierre

Re: [PATCH] mmc: Multi-sector writes

[Russell King]

On Thu, Aug 18, 2005 at 09:26:03AM +0200, Pierre Ossman wrote:
> We had this discussion on LKML and Alan Cox' comment on it was that a
> solution like this would be acceptable, where we try and shove
> everything out first and then fall back on sector-by-sector to determine
> where an error occurs. This will only break if the problematic sector
> keeps shifting around, but at that point the card is probably toast
> anyway (if the thing keeps moving how can you bad block it?).

There are two different kinds of error - the ones at the transport
level which we are able to force a result of "no sectors transferred"
for.  For all other errors and successful completions, the driver
reports "all sectors tranferred" since the driver level doesn't know
that an error occurred.

This causes us to tell the upper levels that we were successful,
even if we weren't.  Hence the problem.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] mmc: Multi-sector writes

[Pierre Ossman]

Russell King wrote:

>On Thu, Aug 18, 2005 at 09:26:03AM +0200, Pierre Ossman wrote:
>  
>
>>We had this discussion on LKML and Alan Cox' comment on it was that a
>>solution like this would be acceptable, where we try and shove
>>everything out first and then fall back on sector-by-sector to determine
>>where an error occurs. This will only break if the problematic sector
>>keeps shifting around, but at that point the card is probably toast
>>anyway (if the thing keeps moving how can you bad block it?).
>>    
>>
>
>There are two different kinds of error - the ones at the transport
>level which we are able to force a result of "no sectors transferred"
>for.  For all other errors and successful completions, the driver
>reports "all sectors tranferred" since the driver level doesn't know
>that an error occurred.
>
>This causes us to tell the upper levels that we were successful,
>even if we weren't.  Hence the problem.
>
>  
>

I still don't understand where you see the problem. As you said there
are two problems that can occur:

* Transport problem. The driver will report back a CRC error, timeout or
whatnot and break. We might not know how many sectors survived so we try
again, going sector-by-sector. We might get a transfer error again,
possibly even before the previous one. But at this point the transport
is probably so noisy that we have little chans of doing a clean umount
anyway. So when the device gets fixed, either by replaying the journal
or doing a fsck, it would have been in the same state if we would have
been able to tell exactly which sectors got written.

* Media error. If the card fails to write data to flash then it will set
the corresponding bits in the status register. We do not check these ATM
so any errors there will get overlooked. So I fail to see how this would
be different when writing several sectors at once. If we implement this
check we will not know where the card failed its write, but again we
fall back to sector-by-sector in this case to determine exactly where
the card has problems.

Rgds
Pierre

Re: [PATCH] mmc: Multi-sector writes

[Pierre Ossman]

Alan Cox wrote:

>On Iau, 2005-08-18 at 09:26 +0200, Pierre Ossman wrote:
>  
>
>>everything out first and then fall back on sector-by-sector to determine
>>where an error occurs. This will only break if the problematic sector
>>keeps shifting around, but at that point the card is probably toast
>>anyway (if the thing keeps moving how can you bad block it?).
>>    
>>
>
>Providing the sectors are not finally completed to higher levels until
>they are written that works fine. 
>

I don't think there's any risk of that. What _might_ happen is that more
data gets written to disk than is reported to the upper layers because
of these buffering issues.

Rgds
Pierre

Re: [PATCH] mmc: Multi-sector writes

[Alan Cox]

On Iau, 2005-08-18 at 09:26 +0200, Pierre Ossman wrote:
> everything out first and then fall back on sector-by-sector to determine
> where an error occurs. This will only break if the problematic sector
> keeps shifting around, but at that point the card is probably toast
> anyway (if the thing keeps moving how can you bad block it?).

Providing the sectors are not finally completed to higher levels until
they are written that works fine. I don't think it matters if the bad
sector goes away either. Providing it has gone away and the real data is
on the media we don't care where the underlying bad block went in the
process and who remapped it.

Alan

Re: [PATCH] mmc: Multi-sector writes

[Pavel Machek]

Hi!

> >>We had this discussion on LKML and Alan Cox' comment on it was that a
> >>solution like this would be acceptable, where we try and shove
> >>everything out first and then fall back on sector-by-sector to determine
> >>where an error occurs. This will only break if the problematic sector
> >>keeps shifting around, but at that point the card is probably toast
> >>anyway (if the thing keeps moving how can you bad block it?).
> >>    
> >>
> >
> >There are two different kinds of error - the ones at the transport
> >level which we are able to force a result of "no sectors transferred"
> >for.  For all other errors and successful completions, the driver
> >reports "all sectors tranferred" since the driver level doesn't know
> >that an error occurred.
> >
> >This causes us to tell the upper levels that we were successful,
> >even if we weren't.  Hence the problem.
> >
> >  
> >
> 
> I still don't understand where you see the problem. As you said there
> are two problems that can occur:
> 
> * Transport problem. The driver will report back a CRC error, timeout or
> whatnot and break. We might not know how many sectors survived so we try
> again, going sector-by-sector. We might get a transfer error again,
> possibly even before the previous one. But at this point the transport
> is probably so noisy that we have little chans of doing a clean umount
> anyway. So when the device gets fixed, either by replaying the journal

Well, but then you can get:

good data #1
trash #2
good data #2
trash #1

I'm not sure how much journalling filesystems will like that in their journals...

-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: [PATCH] mmc: Multi-sector writes

[Pierre Ossman]

Pavel Machek wrote:

>>* Transport problem. The driver will report back a CRC error, timeout or
>>whatnot and break. We might not know how many sectors survived so we try
>>again, going sector-by-sector. We might get a transfer error again,
>>possibly even before the previous one. But at this point the transport
>>is probably so noisy that we have little chans of doing a clean umount
>>anyway. So when the device gets fixed, either by replaying the journal
>>    
>>
>
>Well, but then you can get:
>
>good data #1
>trash #2
>good data #2
>trash #1
>
>I'm not sure how much journalling filesystems will like that in their journals...
>
>  
>

Unless the card is horribly broken it will not write sectors that cannot
be transfered successfully. If there would be a transport error that
goes undetected by the CRC we would just continue, believing that
everything is peachy.

The scenario you're describing could show up in the case of a media
error though. But that would mean that a sector went bad in an extremely
short time, which isn't likely unless the wear leveling has gone crazy
or the card is completely worn out. Either way the card is no longer useful.

Rgds
Pierre

Re: [PATCH] mmc: Multi-sector writes

[Pavel Machek]

Hi!

> >>* Transport problem. The driver will report back a CRC error, timeout or
> >>whatnot and break. We might not know how many sectors survived so we try
> >>again, going sector-by-sector. We might get a transfer error again,
> >>possibly even before the previous one. But at this point the transport
> >>is probably so noisy that we have little chans of doing a clean umount
> >>anyway. So when the device gets fixed, either by replaying the journal
> >>    
> >>
> >
> >Well, but then you can get:
> >
> >good data #1
> >trash #2
> >good data #2
> >trash #1
> >
> >I'm not sure how much journalling filesystems will like that in their journals...
> 
> Unless the card is horribly broken it will not write sectors that cannot
> be transfered successfully. If there would be a transport error that
> goes undetected by the CRC we would just continue, believing that
> everything is peachy.
> 
> The scenario you're describing could show up in the case of a media
> error though. But that would mean that a sector went bad in an extremely
> short time, which isn't likely unless the wear leveling has gone crazy
> or the card is completely worn out. Either way the card is no longer useful.

Maybe the card is pretty close to going to crash, but... two disk
successive disk errors still should not be cause for journal
corruption.

[Also errors could be corelated. Imagine severe overheat. You'll
successive failing writes, but if you let cool it down, you'll still
have working media... only with corrupt journal :-)]
								Pavel
-- 
if you have sharp zaurus hardware you don't need... you know my address

Re: [PATCH] mmc: Multi-sector writes

[Pierre Ossman]

Pavel Machek wrote:

>
>Maybe the card is pretty close to going to crash, but... two disk
>successive disk errors still should not be cause for journal
>corruption.
>
>[Also errors could be corelated. Imagine severe overheat. You'll
>successive failing writes, but if you let cool it down, you'll still
>have working media... only with corrupt journal :-)]
>								Pavel
>  
>

Hmm... So how is this handled in other systems? E.g. if you yank a USB
device whilst there is a lot of outstanding data inside the device that
hasn't been ack:d yet.

The way I see it, filesystems should assume the following at a failed write:

* 0-n sectors were written successfully.
* 0-1 sectors have corrupt data.
* 0-m sectors have old data.
* The lower layer will report back 0-k successfully written sectors,
where k <= n.

So perhaps the best course of action is to remove the sector-by-sector
failsafe? It will increase the chance of k < n, but it will not break
above assumption.

Rgds
Pierre