| Message ID | 1428656543-6790-1-git-send-email-saproj@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2015-04-10 at 11:02 +0200, Sergei Antonov 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(). I think that it really makes sense to describe the issue's reproducing way. It will be really precious for understanding of symptoms and reasons of the issue. Could you add more detailed description? Then, I will have opportunity to test your patch. Thanks, Vyacheslav Dubeyko. > > Cc: Anton Altaparmakov <aia21@cam.ac.uk> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Christoph Hellwig <hch@infradead.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Vyacheslav Dubeyko <slava@dubeyko.com> > Cc: Hin-Tak Leung <htl10@users.sourceforge.net> > Cc: Sougata Santra <sougata@tuxera.com> > Signed-off-by: Sergei Antonov <saproj@gmail.com> > --- > 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); > -out: > bio_put(bio); > return ret < 0 ? ret : 0; > } -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 April 2015 at 18:48, Viacheslav Dubeyko <slava@dubeyko.com> wrote: > On Fri, 2015-04-10 at 11:02 +0200, Sergei Antonov 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(). > > I think that it really makes sense to describe the issue's reproducing > way. It will be really precious for understanding of symptoms and > reasons of the issue. > > Could you add more detailed description? > > Then, I will have opportunity to test your patch. Well, the description says it all. To put it bluntly, when this line from wrapper.c sbi->s_vhdr_buf = kmalloc(hfsplus_min_io_size(sb), GFP_KERNEL); assigns s_vhdr_buf a value satisfying condition (PAGE_SIZE - (value & PAGE_SIZE) < 512) then this call (also from wrapper.c) returns an error: error = hfsplus_submit_bio(sb, part_start + HFSPLUS_VOLHEAD_SECTOR, sbi->s_vhdr_buf, (void **)&sbi->s_vhdr, READ); To give a specific example, sbi->s_vhdr_buf equal to 0xffff8804085acec0 spans two pages and hfsplus_submit_bio() can not read into such a buffer, returns an error, mount operation fails. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7 June 2015 at 22:05, Sergei Antonov <saproj@gmail.com> wrote: > On 10 April 2015 at 18:48, Viacheslav Dubeyko <slava@dubeyko.com> wrote: >> On Fri, 2015-04-10 at 11:02 +0200, Sergei Antonov 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(). >> >> I think that it really makes sense to describe the issue's reproducing >> way. It will be really precious for understanding of symptoms and >> reasons of the issue. >> >> Could you add more detailed description? >> >> Then, I will have opportunity to test your patch. > > Well, the description says it all. To put it bluntly, when this line > from wrapper.c > sbi->s_vhdr_buf = kmalloc(hfsplus_min_io_size(sb), GFP_KERNEL); > assigns s_vhdr_buf a value satisfying condition (PAGE_SIZE - (value & > PAGE_SIZE) < 512) then this call (also from wrapper.c) returns an I'm sorry, the right condition is (PAGE_SIZE - (value & (PAGE_SIZE - 1)) < 512). > error: > error = hfsplus_submit_bio(sb, part_start + HFSPLUS_VOLHEAD_SECTOR, > sbi->s_vhdr_buf, (void **)&sbi->s_vhdr, > READ); > > To give a specific example, sbi->s_vhdr_buf equal to > 0xffff8804085acec0 spans two pages and hfsplus_submit_bio() can not > read into such a buffer, returns an error, mount operation fails. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2015-06-07 at 22:09 +0200, Sergei Antonov wrote: > On 7 June 2015 at 22:05, Sergei Antonov <saproj@gmail.com> wrote: > > On 10 April 2015 at 18:48, Viacheslav Dubeyko <slava@dubeyko.com> wrote: > >> On Fri, 2015-04-10 at 11:02 +0200, Sergei Antonov 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(). > >> > >> I think that it really makes sense to describe the issue's reproducing > >> way. It will be really precious for understanding of symptoms and > >> reasons of the issue. > >> > >> Could you add more detailed description? > >> > >> Then, I will have opportunity to test your patch. > > > > Well, the description says it all. To put it bluntly, when this line > > from wrapper.c > > sbi->s_vhdr_buf = kmalloc(hfsplus_min_io_size(sb), GFP_KERNEL); > > assigns s_vhdr_buf a value satisfying condition (PAGE_SIZE - (value & > > PAGE_SIZE) < 512) then this call (also from wrapper.c) returns an > > I'm sorry, the right condition is (PAGE_SIZE - (value & (PAGE_SIZE - 1)) < 512). > > > error: > > error = hfsplus_submit_bio(sb, part_start + HFSPLUS_VOLHEAD_SECTOR, > > sbi->s_vhdr_buf, (void **)&sbi->s_vhdr, > > READ); > > > > To give a specific example, sbi->s_vhdr_buf equal to > > 0xffff8804085acec0 spans two pages and hfsplus_submit_bio() can not > > read into such a buffer, returns an error, mount operation fails. How an ordinary user can discover this issue? Could you describe a real use-case for the reproducing? Maybe you can share some guess how it can occur? If such situation doesn't take place in the real life then it doesn't make sense to fix it. Please, prove that your fix is valid. Thanks, Vyacheslav Dubeyko. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8 June 2015 at 19:03, Viacheslav Dubeyko <slava@dubeyko.com> wrote: > On Sun, 2015-06-07 at 22:09 +0200, Sergei Antonov wrote: >> On 7 June 2015 at 22:05, Sergei Antonov <saproj@gmail.com> wrote: >> > On 10 April 2015 at 18:48, Viacheslav Dubeyko <slava@dubeyko.com> wrote: >> >> On Fri, 2015-04-10 at 11:02 +0200, Sergei Antonov 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(). >> >> >> >> I think that it really makes sense to describe the issue's reproducing >> >> way. It will be really precious for understanding of symptoms and >> >> reasons of the issue. >> >> >> >> Could you add more detailed description? >> >> >> >> Then, I will have opportunity to test your patch. >> > >> > Well, the description says it all. To put it bluntly, when this line >> > from wrapper.c >> > sbi->s_vhdr_buf = kmalloc(hfsplus_min_io_size(sb), GFP_KERNEL); >> > assigns s_vhdr_buf a value satisfying condition (PAGE_SIZE - (value & >> > PAGE_SIZE) < 512) then this call (also from wrapper.c) returns an >> >> I'm sorry, the right condition is (PAGE_SIZE - (value & (PAGE_SIZE - 1)) < 512). >> >> > error: >> > error = hfsplus_submit_bio(sb, part_start + HFSPLUS_VOLHEAD_SECTOR, >> > sbi->s_vhdr_buf, (void **)&sbi->s_vhdr, >> > READ); >> > >> > To give a specific example, sbi->s_vhdr_buf equal to >> > 0xffff8804085acec0 spans two pages and hfsplus_submit_bio() can not >> > read into such a buffer, returns an error, mount operation fails. > > How an ordinary user can discover this issue? Could you describe a real > use-case for the reproducing? I discovered it by running "mount <partition> <mountpoint>" command. It was working probabilistically: sometimes it mounted the partition, sometimes not. If course, the driver was not specially tweaked, it was just a compiled source code from Linus' git tree. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2015-06-08 at 19:20 +0200, Sergei Antonov wrote: > On 8 June 2015 at 19:03, Viacheslav Dubeyko <slava@dubeyko.com> wrote: > > On Sun, 2015-06-07 at 22:09 +0200, Sergei Antonov wrote: > >> On 7 June 2015 at 22:05, Sergei Antonov <saproj@gmail.com> wrote: > >> > On 10 April 2015 at 18:48, Viacheslav Dubeyko <slava@dubeyko.com> wrote: > >> >> On Fri, 2015-04-10 at 11:02 +0200, Sergei Antonov 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(). > >> >> > >> >> I think that it really makes sense to describe the issue's reproducing > >> >> way. It will be really precious for understanding of symptoms and > >> >> reasons of the issue. > >> >> > >> >> Could you add more detailed description? > >> >> > >> >> Then, I will have opportunity to test your patch. > >> > > >> > Well, the description says it all. To put it bluntly, when this line > >> > from wrapper.c > >> > sbi->s_vhdr_buf = kmalloc(hfsplus_min_io_size(sb), GFP_KERNEL); > >> > assigns s_vhdr_buf a value satisfying condition (PAGE_SIZE - (value & > >> > PAGE_SIZE) < 512) then this call (also from wrapper.c) returns an > >> > >> I'm sorry, the right condition is (PAGE_SIZE - (value & (PAGE_SIZE - 1)) < 512). > >> > >> > error: > >> > error = hfsplus_submit_bio(sb, part_start + HFSPLUS_VOLHEAD_SECTOR, > >> > sbi->s_vhdr_buf, (void **)&sbi->s_vhdr, > >> > READ); > >> > > >> > To give a specific example, sbi->s_vhdr_buf equal to > >> > 0xffff8804085acec0 spans two pages and hfsplus_submit_bio() can not > >> > read into such a buffer, returns an error, mount operation fails. > > > > How an ordinary user can discover this issue? Could you describe a real > > use-case for the reproducing? > > I discovered it by running "mount <partition> <mountpoint>" command. > It was working probabilistically: sometimes it mounted the partition, > sometimes not. If course, the driver was not specially tweaked, it was > just a compiled source code from Linus' git tree. Could you describe your environment with more details? What partition do you have? How did it created? What option did you use for partition creation? What mount option did you use? What Linux version did you use? Thanks, Vyacheslav Dubeyko. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sergei, > On 10 Apr 2015, at 12:02, Sergei Antonov <saproj@gmail.com> 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@cam.ac.uk> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Christoph Hellwig <hch@infradead.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Vyacheslav Dubeyko <slava@dubeyko.com> > Cc: Hin-Tak Leung <htl10@users.sourceforge.net> > Cc: Sougata Santra <sougata@tuxera.com> > Signed-off-by: Sergei Antonov <saproj@gmail.com> > --- > 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 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; > }
On 10 June 2015 at 00:32, Anton Altaparmakov <anton@tuxera.com> wrote: > Hi Sergei, > >> On 10 Apr 2015, at 12:02, Sergei Antonov <saproj@gmail.com> 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@cam.ac.uk> >> Cc: Al Viro <viro@zeniv.linux.org.uk> >> Cc: Christoph Hellwig <hch@infradead.org> >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: Vyacheslav Dubeyko <slava@dubeyko.com> >> Cc: Hin-Tak Leung <htl10@users.sourceforge.net> >> Cc: Sougata Santra <sougata@tuxera.com> >> Signed-off-by: Sergei Antonov <saproj@gmail.com> >> --- >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 10, 2015 at 01:53:47AM +0200, Sergei Antonov wrote: > >> + 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 :). And we're getting right of it because it's problematic. Please allocate an optimistic numbero of vectors in bio_alloc and use bio_add_page as only that takes care of the block driver limits properly. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); -out: bio_put(bio); return ret < 0 ? ret : 0; }
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@cam.ac.uk> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Christoph Hellwig <hch@infradead.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Vyacheslav Dubeyko <slava@dubeyko.com> Cc: Hin-Tak Leung <htl10@users.sourceforge.net> Cc: Sougata Santra <sougata@tuxera.com> Signed-off-by: Sergei Antonov <saproj@gmail.com> --- fs/hfsplus/wrapper.c | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-)