MMC layer regression with single-block controllers

MMC layer regression with single-block controllers

[Manuel Lauss]

Hello Pierre,

Since about 2.6.28-rc1, I hit the following BUG_ON() in mmc/core.c
(line 155):

        if (mrq->data) {
                BUG_ON(mrq->data->blksz > host->max_blk_size);
>>>>>>>>>       BUG_ON(mrq->data->blocks > host->max_blk_count);  <<<<<<
                BUG_ON(mrq->data->blocks * mrq->data->blksz >
                        host->max_req_size);

It's easy to reproduce; just set your favorite host controllers'
mmc->max_blk_cnt to 1 (or anything smaller than 4).  As far as I can
tell it was introduced with some block layer changes before 2.6.28-rc1
was released.

Thanks,
	Manuel Lauss

Re: MMC layer regression with single-block controllers

[Pierre Ossman]

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

On Mon, 23 Mar 2009 10:28:02 +0100
Manuel Lauss <mano@roarinelk.homelinux.net> wrote:

> Hello Pierre,
> 
> Since about 2.6.28-rc1, I hit the following BUG_ON() in mmc/core.c
> (line 155):
> 
>         if (mrq->data) {
>                 BUG_ON(mrq->data->blksz > host->max_blk_size);
> >>>>>>>>>       BUG_ON(mrq->data->blocks > host->max_blk_count);  <<<<<<
>                 BUG_ON(mrq->data->blocks * mrq->data->blksz >
>                         host->max_req_size);
> 
> It's easy to reproduce; just set your favorite host controllers'
> mmc->max_blk_cnt to 1 (or anything smaller than 4).  As far as I can
> tell it was introduced with some block layer changes before 2.6.28-rc1
> was released.
> 

Ouch. Have you determined that it is the block layer that isn't
respecting its settings? If so, the we probably need to notify Jens
Axboe.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: MMC layer regression with single-block controllers

[Manuel Lauss]

Hello Pierre, Jens,

On Tue, Mar 24, 2009 at 09:01:38PM +0100, Pierre Ossman wrote:
> On Mon, 23 Mar 2009 10:28:02 +0100
> Manuel Lauss <mano@roarinelk.homelinux.net> wrote:
> 
> > Hello Pierre,
> > 
> > Since about 2.6.28-rc1, I hit the following BUG_ON() in mmc/core.c
> > (line 155):
> > 
> >         if (mrq->data) {
> >                 BUG_ON(mrq->data->blksz > host->max_blk_size);
> > >>>>>>>>>       BUG_ON(mrq->data->blocks > host->max_blk_count);  <<<<<<
> >                 BUG_ON(mrq->data->blocks * mrq->data->blksz >
> >                         host->max_req_size);
> > 
> > It's easy to reproduce; just set your favorite host controllers'
> > mmc->max_blk_cnt to 1 (or anything smaller than 4).  As far as I can
> > tell it was introduced with some block layer changes before 2.6.28-rc1
> > was released.
> > 
> 
> Ouch. Have you determined that it is the block layer that isn't
> respecting its settings? If so, the we probably need to notify Jens
> Axboe.

(Added Jens to CC).

Yes, it seems the block layer doesn't honor the one-sector request made
in drivers/mmc/card/queue.c:187 (and 161).  The debug log says:

mmc0: new SD card at address aaaa
blk_queue_max_sectors: set to minimum 8
mmc0: starting CMD16 arg 00000200 flags 00000095
mmc0: req done (CMD16): 0: 00000900 00000000 00000000 00000000
mmcblk0: mmc0:aaaa SD01G 968 MiB
 mmcblk0:<7>mmc0: starting CMD18 arg 00000000 flags 000000b5
mmc0:     blksz 512 blocks 8 flags 00000200 tsac 100 ms nsac 0
mmc0:     CMD12 arg 00000000 flags 0000049d
------------[ cut here ]------------
kernel BUG at /mnt/work/sh7760/kernel/linux-2.6.git/drivers/mmc/core/core.c:155!


This "blk_queue_max_sectors: set to minimum 8" is what's haunting my
mmc host driver (hardware has enormous problems doing multiblock writes,
hence the limitation to a single block per request).

Thanks!
	Manuel Lauss

Re: MMC layer regression with single-block controllers

[Jens Axboe]

On Wed, Mar 25 2009, Manuel Lauss wrote:
> Hello Pierre, Jens,
> 
> On Tue, Mar 24, 2009 at 09:01:38PM +0100, Pierre Ossman wrote:
> > On Mon, 23 Mar 2009 10:28:02 +0100
> > Manuel Lauss <mano@roarinelk.homelinux.net> wrote:
> > 
> > > Hello Pierre,
> > > 
> > > Since about 2.6.28-rc1, I hit the following BUG_ON() in mmc/core.c
> > > (line 155):
> > > 
> > >         if (mrq->data) {
> > >                 BUG_ON(mrq->data->blksz > host->max_blk_size);
> > > >>>>>>>>>       BUG_ON(mrq->data->blocks > host->max_blk_count);  <<<<<<
> > >                 BUG_ON(mrq->data->blocks * mrq->data->blksz >
> > >                         host->max_req_size);
> > > 
> > > It's easy to reproduce; just set your favorite host controllers'
> > > mmc->max_blk_cnt to 1 (or anything smaller than 4).  As far as I can
> > > tell it was introduced with some block layer changes before 2.6.28-rc1
> > > was released.
> > > 
> > 
> > Ouch. Have you determined that it is the block layer that isn't
> > respecting its settings? If so, the we probably need to notify Jens
> > Axboe.
> 
> (Added Jens to CC).
> 
> Yes, it seems the block layer doesn't honor the one-sector request made
> in drivers/mmc/card/queue.c:187 (and 161).  The debug log says:
> 
> mmc0: new SD card at address aaaa
> blk_queue_max_sectors: set to minimum 8
> mmc0: starting CMD16 arg 00000200 flags 00000095
> mmc0: req done (CMD16): 0: 00000900 00000000 00000000 00000000
> mmcblk0: mmc0:aaaa SD01G 968 MiB
>  mmcblk0:<7>mmc0: starting CMD18 arg 00000000 flags 000000b5
> mmc0:     blksz 512 blocks 8 flags 00000200 tsac 100 ms nsac 0
> mmc0:     CMD12 arg 00000000 flags 0000049d
> ------------[ cut here ]------------
> kernel BUG at /mnt/work/sh7760/kernel/linux-2.6.git/drivers/mmc/core/core.c:155!
> 
> 
> This "blk_queue_max_sectors: set to minimum 8" is what's haunting my
> mmc host driver (hardware has enormous problems doing multiblock writes,
> hence the limitation to a single block per request).

Well, as the warning tells you, it's not supported. But that doesn't
mean you can't do single sector transfers and complete the IO when you
have done all of them.

-- 
Jens Axboe

Re: MMC layer regression with single-block controllers

[Pierre Ossman]

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

On Wed, 25 Mar 2009 12:04:01 +0100
Jens Axboe <jens.axboe@oracle.com> wrote:

> 
> Well, as the warning tells you, it's not supported. But that doesn't
> mean you can't do single sector transfers and complete the IO when you
> have done all of them.
> 

That's an annoying limitation and sort of defeats the purpose of
specifying a maximum sector count. Why can't it go below 8?

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: MMC layer regression with single-block controllers

[Jens Axboe]

On Wed, Mar 25 2009, Pierre Ossman wrote:
> On Wed, 25 Mar 2009 12:04:01 +0100
> Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > 
> > Well, as the warning tells you, it's not supported. But that doesn't
> > mean you can't do single sector transfers and complete the IO when you
> > have done all of them.
> > 
> 
> That's an annoying limitation and sort of defeats the purpose of
> specifying a maximum sector count. Why can't it go below 8?

Because it would add all sorts of complexity for handling 4kb block size
file systems on top of the block device. The bio_add_page() api depends
on being able to add a single page at least, it has to or you would need
to push that complexity to the callers.

So it's annoying, but you have to do the incremental completions
yourself for the (obscure) device that doesn't do more than 512b
transfers. It's really not that difficult, and it would be simpler than
supporting partial completions generically in the block layer.

Saying that it defeats the purpose of the setting is missing the bigger
picture - basically all other devices, that have sane limits (like 128k,
or whatnot).

-- 
Jens Axboe

Re: MMC layer regression with single-block controllers

[Pierre Ossman]

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

On Wed, 25 Mar 2009 12:42:48 +0100
Jens Axboe <jens.axboe@oracle.com> wrote:

> On Wed, Mar 25 2009, Pierre Ossman wrote:
> > 
> > That's an annoying limitation and sort of defeats the purpose of
> > specifying a maximum sector count. Why can't it go below 8?
> 
> Because it would add all sorts of complexity for handling 4kb block size
> file systems on top of the block device. The bio_add_page() api depends
> on being able to add a single page at least, it has to or you would need
> to push that complexity to the callers.
> 

Requests going in into the block layer and requests coming out aren't
that strongly coupled, so I don't see it of that much of a stretch to
expect this to be handled.

> So it's annoying, but you have to do the incremental completions
> yourself for the (obscure) device that doesn't do more than 512b
> transfers. It's really not that difficult, and it would be simpler than
> supporting partial completions generically in the block layer.

The code was there previously, but it seemed a bit redundant to have
functionality like that in the block driver since we've already told
the block layer about the restrictions.

The code was pretty simple. Basically it just cropped the sg list at
the correct place. Couldn't that be as easily done in the block layer?

> 
> Saying that it defeats the purpose of the setting is missing the bigger
> picture - basically all other devices, that have sane limits (like 128k,
> or whatnot).
> 

I suppose. But it makes the feature completely useless for MMC as it
has to implement it's own cropping of requests anyway.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: MMC layer regression with single-block controllers

[Jens Axboe]

On Wed, Mar 25 2009, Pierre Ossman wrote:
> On Wed, 25 Mar 2009 12:42:48 +0100
> Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > On Wed, Mar 25 2009, Pierre Ossman wrote:
> > > 
> > > That's an annoying limitation and sort of defeats the purpose of
> > > specifying a maximum sector count. Why can't it go below 8?
> > 
> > Because it would add all sorts of complexity for handling 4kb block size
> > file systems on top of the block device. The bio_add_page() api depends
> > on being able to add a single page at least, it has to or you would need
> > to push that complexity to the callers.
> > 
> 
> Requests going in into the block layer and requests coming out aren't
> that strongly coupled, so I don't see it of that much of a stretch to
> expect this to be handled.

The point is that you have to handle at one side of the block layer, at
submission or at send-to-driver time.

> > So it's annoying, but you have to do the incremental completions
> > yourself for the (obscure) device that doesn't do more than 512b
> > transfers. It's really not that difficult, and it would be simpler than
> > supporting partial completions generically in the block layer.
> 
> The code was there previously, but it seemed a bit redundant to have
> functionality like that in the block driver since we've already told
> the block layer about the restrictions.

You never saw the warnings? It's pretty clear that it does not support <
PAGE_CACHE_SIZE blocks. It has always been so, I don't know why the
subject says regression. I guess that is referring to a mmc layer
regression?

> The code was pretty simple. Basically it just cropped the sg list at
> the correct place. Couldn't that be as easily done in the block layer?

No, because if you do it transparently, then you have to keep partial
state in the bio for completions. So it makes everything a lot more
complex, I don't want to do that for something like this.

> > Saying that it defeats the purpose of the setting is missing the bigger
> > picture - basically all other devices, that have sane limits (like 128k,
> > or whatnot).
> > 
> 
> I suppose. But it makes the feature completely useless for MMC as it
> has to implement it's own cropping of requests anyway.

Unfortunate, it's useful for other cases though.

-- 
Jens Axboe

Re: MMC layer regression with single-block controllers

[Pierre Ossman]

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

On Wed, 25 Mar 2009 17:14:25 +0100
Jens Axboe <jens.axboe@oracle.com> wrote:

> On Wed, Mar 25 2009, Pierre Ossman wrote:
> > 
> > The code was there previously, but it seemed a bit redundant to have
> > functionality like that in the block driver since we've already told
> > the block layer about the restrictions.
> 
> You never saw the warnings? It's pretty clear that it does not support <
> PAGE_CACHE_SIZE blocks. It has always been so, I don't know why the
> subject says regression. I guess that is referring to a mmc layer
> regression?
> 

Yes. The MMC code did all of this magic by itself previously and
assumed very little about the block layer.

> > The code was pretty simple. Basically it just cropped the sg list at
> > the correct place. Couldn't that be as easily done in the block layer?
> 
> No, because if you do it transparently, then you have to keep partial
> state in the bio for completions. So it makes everything a lot more
> complex, I don't want to do that for something like this.
> 

How is this different from the low level driver partially completing a
request, which is how it would have to be handled otherwise?

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: MMC layer regression with single-block controllers

[Jens Axboe]

On Wed, Mar 25 2009, Pierre Ossman wrote:
> On Wed, 25 Mar 2009 17:14:25 +0100
> Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > On Wed, Mar 25 2009, Pierre Ossman wrote:
> > > 
> > > The code was there previously, but it seemed a bit redundant to have
> > > functionality like that in the block driver since we've already told
> > > the block layer about the restrictions.
> > 
> > You never saw the warnings? It's pretty clear that it does not support <
> > PAGE_CACHE_SIZE blocks. It has always been so, I don't know why the
> > subject says regression. I guess that is referring to a mmc layer
> > regression?
> > 
> 
> Yes. The MMC code did all of this magic by itself previously and
> assumed very little about the block layer.
> 
> > > The code was pretty simple. Basically it just cropped the sg list at
> > > the correct place. Couldn't that be as easily done in the block layer?
> > 
> > No, because if you do it transparently, then you have to keep partial
> > state in the bio for completions. So it makes everything a lot more
> > complex, I don't want to do that for something like this.
> > 
> 
> How is this different from the low level driver partially completing a
> request, which is how it would have to be handled otherwise?

It's a completely parallel issue. We keep extra state to handle the
partial completion bits, we would need to add even more state to handle
presenting a smaller view of the request (and the bio, it would need to
work for both) to the driver.

-- 
Jens Axboe