BUG in the block layer (partial reads not reported)

BUG in the block layer (partial reads not reported)

[Alan Stern]

The block layer does not report partial reads correctly back to userspace.

Here's an example which I can replicate at will.  This is using the SCSI 
cdrom driver and a disc with 326535 hardware sectors, each containing 2048 
bytes.  The last two sectors are unreadable.

Now consider what should happen when you run

	dd if=/dev/scd0 bs=2048 count=1 skip=326532

One would expect to get back the contents of the last readable hardware 
sector.

Instead, what happens is this:

     1. The block layer issues a read for sectors 326532-3 (i.e., a 
	page's worth, including the sector we want and the following
	unreadable sector).

     2. The read partially succeeds, and the driver calls 
		end_that_request_chunk(req, 1, 2048);
	It then requeues the request, more or less by coincidence.

     3. This time the request fails since it's trying to read the
	second-to-last sector, and the driver calls
		end_that_request_chunk(req, 1, 0);
	I'm not sure why.

     4. Then the driver does what it should have done before, and calls
		end_that_request_chunk(req, 0, 2048);
	This causes an I/O error message to appear in the system log.

     5. The driver calls end_that_request_last(req).

     6. Apparently the block layer issues its own retry at this point.
	The driver gets another read request for sector 326533.

     7. Steps 3 - 5 repeat.

The end result is that dd receives no data, only an error.  This is in 
spite of the fact that the kernel was able to read successfully all the 
data that had been requested!

Now I have only the vaguest notion of how the block layer works, and I 
don't know where to begin solving this problem.  Any help would be 
appreciated.

Alan Stern

Re: BUG in the block layer (partial reads not reported)

[Alan Cox]

On Iau, 2005-10-20 at 16:39 -0400, Alan Stern wrote:
> The end result is that dd receives no data, only an error.  This is in 
> spite of the fact that the kernel was able to read successfully all the 
> data that had been requested!
> 
> Now I have only the vaguest notion of how the block layer works, and I 
> don't know where to begin solving this problem.  Any help would be 
> appreciated.


Yes I found this with the IDE layer work - I reported it last year. There is
no mechanism to return EOF through the block layer.

The driver behaviour you describe appears correct except for the (as far
as I can see harmless) 0 byte chunk.

It reports the first sector as correct and completed. It then reports
the second sector as failed and an error. It then completes the request.

The block layer then retries (because there is no mechanism to report
EOF upstream), this fails and the block layer is left with a page that
contains 2048 bytes of valid data and 2048 bytes of undefined.
Unfortunately it doesn't really know what do with this.

If you modify the drivers to fill the remaining 2048 bytes of the buffer
with "JENSAXBOEPLEASEFIXME" or something similar and hand it up as
completed instead of an error when the EOF case is hit that'll sort of
do the right thing. Of course that assumes you can tell its an EOF but
that will be the case even if the EOF handling is fixed.

Sort of related to this is that handling of read-ahead triggered errors
is broken when the span a page because the kernel doesn't really know
how to deal with a page of memory which is half valid and remember that.
Thus accessing the page which contains the 2K of data you needed to save
and the error block will give you an unneccessary error return and no
data. The block layer ought to retry the I/O in this case and handle a
short return correctly but it still seems not to.

The essential problem however is that if you say a disc is a given size
and it turns out not to be (as happens with CD-R especially or with
buggy readers) then Linux block layer can't cope. Its well known and
causes endless problems for CD-R users with some IDE drives on Linux.
Its a big generator of 2.6 vendor bug reports.

In the case where the media size is correctly set you should find the
block layer never reads ahead beyond the declared EOF.

Alan

Re: BUG in the block layer (partial reads not reported)

[Alan Stern]

On Fri, 21 Oct 2005, Alan Cox wrote:

> The block layer then retries (because there is no mechanism to report
> EOF upstream), this fails and the block layer is left with a page that
> contains 2048 bytes of valid data and 2048 bytes of undefined.
> Unfortunately it doesn't really know what do with this.
> 
> If you modify the drivers to fill the remaining 2048 bytes of the buffer
> with "JENSAXBOEPLEASEFIXME" or something similar and hand it up as
> completed instead of an error when the EOF case is hit that'll sort of
> do the right thing. Of course that assumes you can tell its an EOF but
> that will be the case even if the EOF handling is fixed.
> 
> Sort of related to this is that handling of read-ahead triggered errors
> is broken when the span a page because the kernel doesn't really know
> how to deal with a page of memory which is half valid and remember that.
> Thus accessing the page which contains the 2K of data you needed to save
> and the error block will give you an unneccessary error return and no
> data. The block layer ought to retry the I/O in this case and handle a
> short return correctly but it still seems not to.
> 
> The essential problem however is that if you say a disc is a given size
> and it turns out not to be (as happens with CD-R especially or with
> buggy readers) then Linux block layer can't cope. Its well known and
> causes endless problems for CD-R users with some IDE drives on Linux.
> Its a big generator of 2.6 vendor bug reports.
> 
> In the case where the media size is correctly set you should find the
> block layer never reads ahead beyond the declared EOF.

I gather then that there are really two separate problems:

	Handling an error somewhere in the middle of the medium, and

	Handling an error beyond the real end of the medium.

The mm and block subsystems have no way at all to retrieve partial data
for the first case.  Even though only one hardware sector may be bad,
failure to read an entire page means that none of the good sectors on that
page will be accessible.  While annoying, it's understandable and I don't 
see any simple way to accomodate such partial reads.

The second case appears to be more tractable, as you said.  In fact,
do_generic_mapping_read() in mm/filemap.c will recheck the inode's size
after a successful read, to avoid copying data beyond the end of the
device.

Could part of the problem also be that the set_capacity() call, used to
revise the device size downward when the CD driver realizes it is smaller
than originally thought, doesn't update the inode?  Should the driver call
bd_set_size() as well?  (In addition to completing the read successfully
with garbage data beyond the actual EOF.)

Incidentally, can you explain what bd_block_size in struct block_device is 
for?  Is the block_size used for anything important, that is, more 
important than the hardware sector size?  I ask because of the strange 
code in bd_set_size which sets bd_block_size equal to the largest power of 
2 that evenly divides the device size and is <= the PAGE_CACHE_SIZE.

Alan Stern

Re: BUG in the block layer (partial reads not reported)

[Alan Cox]

On Sad, 2005-10-22 at 11:40 -0400, Alan Stern wrote:
> 	Handling an error somewhere in the middle of the medium, and
> 
> 	Handling an error beyond the real end of the medium.
> 
> The mm and block subsystems have no way at all to retrieve partial data
> for the first case.  Even though only one hardware sector may be bad,

The block layer can handle this at the bottom level but the caches above
it cannot. 

> failure to read an entire page means that none of the good sectors on that
> page will be accessible.  While annoying, it's understandable and I don't 
> see any simple way to accomodate such partial reads.

Agreed it is hairy with things like mmap. One way is to use raw I/O and
disable readahead.

> The second case appears to be more tractable, as you said.  In fact,
> do_generic_mapping_read() in mm/filemap.c will recheck the inode's size
> after a successful read, to avoid copying data beyond the end of the
> device.
> 
> Could part of the problem also be that the set_capacity() call, used to
> revise the device size downward when the CD driver realizes it is smaller
> than originally thought, doesn't update the inode?  Should the driver call
> bd_set_size() as well?  (In addition to completing the read successfully
> with garbage data beyond the actual EOF.)

Beats me. Perhaps Jens can enlighten us and I can improve the ide-cd
driver code further as well.

Re: BUG in the block layer (partial reads not reported)

[Alan Stern]

On Sat, 22 Oct 2005, Alan Cox wrote:

> On Sad, 2005-10-22 at 11:40 -0400, Alan Stern wrote:
> > 	Handling an error somewhere in the middle of the medium, and
> > 
> > 	Handling an error beyond the real end of the medium.
> > 
> > The mm and block subsystems have no way at all to retrieve partial data
> > for the first case.  Even though only one hardware sector may be bad,
> 
> The block layer can handle this at the bottom level but the caches above
> it cannot. 

The problem is apparent when one reads end_buffer_async_read() in
fs/buffer.c.  If any of the buffer_heads in a page gets an error, the 
entire page is marked with SetPageError.

> > failure to read an entire page means that none of the good sectors on that
> > page will be accessible.  While annoying, it's understandable and I don't 
> > see any simple way to accomodate such partial reads.
> 
> Agreed it is hairy with things like mmap. One way is to use raw I/O and
> disable readahead.
> 
> > The second case appears to be more tractable, as you said.  In fact,
> > do_generic_mapping_read() in mm/filemap.c will recheck the inode's size
> > after a successful read, to avoid copying data beyond the end of the
> > device.
> > 
> > Could part of the problem also be that the set_capacity() call, used to
> > revise the device size downward when the CD driver realizes it is smaller
> > than originally thought, doesn't update the inode?  Should the driver call
> > bd_set_size() as well?  (In addition to completing the read successfully
> > with garbage data beyond the actual EOF.)
> 
> Beats me. Perhaps Jens can enlighten us and I can improve the ide-cd
> driver code further as well.

It turns out I was looking in the wrong place.  do_generic_mapping_read() 
merely initiates the I/O; the completion happens somewhere else.

Shrinking the inode size probably is not a good idea.  There may already 
be pages mapped to sectors beyond the true end of the device.  We can't 
just leave them hanging out to dry.  (But what happens when a media eject 
is detected while the device file is open -- how is that handled?)

At the moment I can think of just two possibilities:

    (1) We can continue to do what the SCSI CD driver does now.  Reads
	near the end of the medium might fail.  When the device is closed
	and then re-opened, the correct size will be installed in the
	inode and so everything will work.

    (2) We can return success for reads beyond the end of the medium
	(and fill the buffers with 0s).  The first time the device is
	opened, the user will then be able to read all the way up to
	(and perhaps a little beyond!) the end of the medium.

The advantage of (2) is that all the actual data is accessible all the 
time.  With (1), the last sector or so might not be accessible until the 
device is closed and re-opened.

Either way, it doesn't seem possible to do exactly the right thing.  Maybe 
there is a way to do it, though, and I simply don't know what it is.

Alan Stern

Re: BUG in the block layer (partial reads not reported)

[Helge Hafting]

On Fri, Oct 21, 2005 at 06:31:57PM +0100, Alan Cox wrote:
> 
> The essential problem however is that if you say a disc is a given size
> and it turns out not to be (as happens with CD-R especially or with
> buggy readers) then Linux block layer can't cope. Its well known and
> causes endless problems for CD-R users with some IDE drives on Linux.
> Its a big generator of 2.6 vendor bug reports.
> 
Seems to me that the best fix for devices that may re�port the wrong size
is to always use a foolproof way of determining the size.  I.e. when
a CD-R cannot be trusted, determine the size by trying to read the
last sectors instead of using the reported number.  

Helge Hafting

Re: BUG in the block layer (partial reads not reported)

[Alan Cox]

On Llu, 2005-10-24 at 00:12 +0200, Helge Hafting wrote:
> Seems to me that the best fix for devices that may re�port the wrong size
> is to always use a foolproof way of determining the size.  I.e. when
> a CD-R cannot be trusted, determine the size by trying to read the
> last sectors instead of using the reported number.  

That may take up to half a minute on some CD drives that retry a lot