On 10 June 2015 at 00:32, Anton Altaparmakov <anton@xxxxxxxxxx> wrote:
> Hi Sergei,
>
>> On 10 Apr 2015, at 12:02, Sergei Antonov <saproj@xxxxxxxxx> wrote:
>>
>> Function hfsplus_submit_bio() did not work when the passed buffer spanned
>> over more than one page. That was because bio_alloc() is passed 1 as a number
>> of vectors but more than one vector were added inside the 'while' loop.
>> It periodically caused a mount error when the volume header could not be read.
>>
>> This patch modifies the code so that only one vector is used. It works for
>> multiple pages too. Also adds a return code check after bio_alloc().
>>
>> Cc: Anton Altaparmakov <aia21@xxxxxxxxx>
>> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
>> Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Vyacheslav Dubeyko <slava@xxxxxxxxxxx>
>> Cc: Hin-Tak Leung <htl10@xxxxxxxxxxxxxxxxxxxxx>
>> Cc: Sougata Santra <sougata@xxxxxxxxxx>
>> Signed-off-by: Sergei Antonov <saproj@xxxxxxxxx>
>> ---
>> fs/hfsplus/wrapper.c | 29 ++++++++++-------------------
>> 1 file changed, 10 insertions(+), 19 deletions(-)
>>
>> diff --git a/fs/hfsplus/wrapper.c b/fs/hfsplus/wrapper.c
>> index cc62356..e245faa 100644
>> --- a/fs/hfsplus/wrapper.c
>> +++ b/fs/hfsplus/wrapper.c
>> @@ -62,29 +62,20 @@ int hfsplus_submit_bio(struct super_block *sb, sector_t sector,
>> offset = start & (io_size - 1);
>> sector &= ~((io_size >> HFSPLUS_SECTOR_SHIFT) - 1);
>>
>> - bio = bio_alloc(GFP_NOIO, 1);
>> - bio->bi_iter.bi_sector = sector;
>> - bio->bi_bdev = sb->s_bdev;
>> -
>> if (!(rw & WRITE) && data)
>> *data = (u8 *)buf + offset;
>>
>> - while (io_size > 0) {
>> - unsigned int page_offset = offset_in_page(buf);
>> - unsigned int len = min_t(unsigned int, PAGE_SIZE - page_offset,
>> - io_size);
>> -
>> - ret = bio_add_page(bio, virt_to_page(buf), len, page_offset);
>> - if (ret != len) {
>> - ret = -EIO;
>> - goto out;
>> - }
>> - io_size -= len;
>> - buf = (u8 *)buf + len;
>> - }
>> -
>> + bio = bio_alloc(GFP_NOIO, 1);
>> + if (!bio)
>> + return -ENOMEM;
>> + bio->bi_iter.bi_sector = sector;
>> + bio->bi_bdev = sb->s_bdev;
>> + bio->bi_vcnt = 1;
>> + bio->bi_iter.bi_size = io_size;
>> + bio->bi_io_vec[0].bv_page = virt_to_page(buf);
>> + bio->bi_io_vec[0].bv_offset = offset_in_page(buf);
>> + bio->bi_io_vec[0].bv_len = io_size;
>> ret = submit_bio_wait(rw, bio);
>
> I think you need to rethink this. Think of what you are doing: You are adding a single page to the bio but then submitting io that exceeds the page...
I'm not adding a single page, I'm adding a single vector.
This "bio->bi_vcnt = 1; bio->bi_io_vec[0].bv_page = ...; etc."
approach can be found in a number of places in the kernel. Just
followed other people's example. And I tested the code too :).
> I think you should fix it the other way round, i.e. use the original code but increase the number passed to bio_alloc() to:
>
> bio = bio_alloc(GFP_NOIO, (offset_in_page(buf) + io_size + PAGE_CACHE_SIZE - 1) / PAGE_CACHE_SIZE);
>
> Or if you know that it can only ever span two pages (you would need to check all call sites of hfsplus_submit_bio() which I have not done then you could potentially just use a constant 2, i.e.
>
> bio = bio_alloc(GFP_NOIO, 2);
>
> And then it does not matter whether you add one or two pages it will always work. But don't do that without checking all call sites. If it can be called with a larger number of pages then the above more accurate version might be better.
>
> Also note that if the number can actually grow large then you will need to break it up. bio_alloc() has a limit of BIO_MAX_PAGES so you cannot call it for more than that (or you will just get returned NULL without anything else happening).
>
> Finally, checking the bio_alloc() for failure is obviously a good idea so do keep that change.
>
> Best regards,
>
> Anton
>
>> -out:
>> bio_put(bio);
>> return ret < 0 ? ret : 0;
>> }
>
> --
> Anton Altaparmakov <anton at tuxera.com> (replace at with @)
> Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
> Linux NTFS maintainer
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
