bcache should support reads shorter than sector size

[tsoome]

Summary

dosfs (fat file systems) can perform reads for few bytes, bcache should support such reads.

Test Plan

browsing/reading dosfs from loader.

[cem]

I don't think the message matches the diff?

The diff appears to truncate bcopied data for reads where some of the trailing blocks were not populated in to the cache with dv_strategy.

I.e., this appears to fix a logic bug that previously existed in the "check how much data can we copy" code, but does not appear to have anything to do with msdosfs or fragments.

[tsoome]

In D6475#137183, @cem wrote:

I don't think the message matches the diff?

The diff appears to truncate bcopied data for reads where some of the trailing blocks were not populated in to the cache with dv_strategy.

I.e., this appears to fix a logic bug that previously existed in the "check how much data can we copy" code, but does not appear to have anything to do with msdosfs or fragments.

I guess the wording is not the best one, true:D but actually it has everything to do about the dosfs, because that one is the only consumer issuing the custom size reads (well, except if someone will write such code on purpose). And this is the reason why the tests did not reveal the problem till yesterday when I was working with fat32.

I think perhaps more fitting description would be that bcache read is corrupting memory by reading more data than requested - which is exactly what happens with reads from dosfs with size < 512 bytes.

[cem]

Yes, that wording is better.

Still, I don't understand how this case happens. nblk is simply size / bcache_blksize, rounded up to 1; i is at most nblk; how can it possibly be the case that size > i * bcache_blksize (unless dv_strategy failed to read some blocks or we hit end of disk)? How does FAT affect this?

In the size < 512 case, assuming bcache_blksize is 512, size > 1 * 512 will not be true because size is less than 512.

[tsoome]

In D6475#137196, @cem wrote:

Yes, that wording is better.

Still, I don't understand how this case happens. nblk is simply size / bcache_blksize, rounded up to 1; i is at most nblk; how can it possibly be the case that size > i * bcache_blksize (unless dv_strategy failed to read some blocks or we hit end of disk)? How does FAT affect this?

FAT affects it by issuing < 512B reads *and* with exact buffer for such reads.

if size < 512, we will have nblk 1, yes, so the code did fill the return buffer with 512 bytes. Also, unfortunately, writing past buffer end by 512 - size bytes *if* the provided buffer was allocated for exactly size bytes; also as an side effect, the while(size) loop in bcache_strategy() will fail to terminate, because the amount of data read is reported back 512 and the value of size in bcache_strategy() got wrapped.. Essentially the issue is, bcache code was ok for n*512 byte reads, with this patch it should be also ok for n byte reads. Well, as much as reading dosfs is working, we can pretty much say it is ok for N byte reads.

[cem]

Oh, I was missing the removed line before. Doh! Now the change makes sense.

This is essentially size = MIN(size, i * bcache_blksize). Okay.