| Message ID | 20190625024625.23976-2-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix zone revalidation memory allocation failures | expand |
Looks good with one nit, can be done at the time of applying patch. Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> On 6/24/19 7:46 PM, Damien Le Moal wrote: > To allow the SCSI subsystem scsi_execute_req() function to issue > requests using large buffers that are better allocated with vmalloc() > rather than kmalloc(), modify bio_map_kern() to allow passing a buffer > allocated with the vmalloc() function. To do so, simply test the buffer > address using is_vmalloc_addr() and use vmalloc_to_page() instead of > virt_to_page() to obtain the pages of vmalloc-ed buffers. > > Fixes: 515ce6061312 ("scsi: sd_zbc: Fix sd_zbc_report_zones() buffer allocation") > Fixes: e76239a3748c ("block: add a report_zones method") > Cc: stable@vger.kernel.org > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > block/bio.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/block/bio.c b/block/bio.c > index ce797d73bb43..05afcaf655f3 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -1501,6 +1501,8 @@ struct bio *bio_map_kern(struct request_queue *q, void *data, unsigned int len, > unsigned long end = (kaddr + len + PAGE_SIZE - 1) >> PAGE_SHIFT; > unsigned long start = kaddr >> PAGE_SHIFT; > const int nr_pages = end - start; > + bool is_vmalloc = is_vmalloc_addr(data); > + struct page *page; > int offset, i; > struct bio *bio; > > @@ -1518,7 +1520,11 @@ struct bio *bio_map_kern(struct request_queue *q, void *data, unsigned int len, > if (bytes > len) > bytes = len; > > - if (bio_add_pc_page(q, bio, virt_to_page(data), bytes, > + if (is_vmalloc) nit:- Can we use is_vmalloc_addr() call directly so that "if (is_vmalloc)" -> "if (is_vmalloc_addr(data))" and remove is_vmalloc variable. > + page = vmalloc_to_page(data); > + else > + page = virt_to_page(data); > + if (bio_add_pc_page(q, bio, page, bytes, > offset) < bytes) { > /* we don't support partial mappings */ > bio_put(bio); >
On 6/24/19 8:24 PM, Chaitanya Kulkarni wrote: > nit:- Can we use is_vmalloc_addr() call directly so that > "if (is_vmalloc)" -> "if (is_vmalloc_addr(data))" and remove is_vmalloc > variable. That would change a single call of is_vmalloc_addr() into multiple? Bart.
On 6/24/19 7:46 PM, Damien Le Moal wrote: > To allow the SCSI subsystem scsi_execute_req() function to issue > requests using large buffers that are better allocated with vmalloc() > rather than kmalloc(), modify bio_map_kern() to allow passing a buffer > allocated with the vmalloc() function. To do so, simply test the buffer > address using is_vmalloc_addr() and use vmalloc_to_page() instead of > virt_to_page() to obtain the pages of vmalloc-ed buffers. > > Fixes: 515ce6061312 ("scsi: sd_zbc: Fix sd_zbc_report_zones() buffer allocation") > Fixes: e76239a3748c ("block: add a report_zones method") > Cc: stable@vger.kernel.org > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > block/bio.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/block/bio.c b/block/bio.c > index ce797d73bb43..05afcaf655f3 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -1501,6 +1501,8 @@ struct bio *bio_map_kern(struct request_queue *q, void *data, unsigned int len, > unsigned long end = (kaddr + len + PAGE_SIZE - 1) >> PAGE_SHIFT; > unsigned long start = kaddr >> PAGE_SHIFT; > const int nr_pages = end - start; > + bool is_vmalloc = is_vmalloc_addr(data); > + struct page *page; > int offset, i; > struct bio *bio; > > @@ -1518,7 +1520,11 @@ struct bio *bio_map_kern(struct request_queue *q, void *data, unsigned int len, > if (bytes > len) > bytes = len; > > - if (bio_add_pc_page(q, bio, virt_to_page(data), bytes, > + if (is_vmalloc) > + page = vmalloc_to_page(data); > + else > + page = virt_to_page(data); > + if (bio_add_pc_page(q, bio, page, bytes, > offset) < bytes) { > /* we don't support partial mappings */ > bio_put(bio); Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On 06/25/2019 08:59 AM, Bart Van Assche wrote: > On 6/24/19 8:24 PM, Chaitanya Kulkarni wrote: >> nit:- Can we use is_vmalloc_addr() call directly so that >> "if (is_vmalloc)" -> "if (is_vmalloc_addr(data))" and remove is_vmalloc >> variable. > That would change a single call of is_vmalloc_addr() into multiple? Well is_vmalloc_addr() it is an in-line helper with address comparison. is it too expensive to have such a comparison in the loop ? > > Bart. >
On Tue, Jun 25, 2019 at 11:46:23AM +0900, Damien Le Moal wrote: > To allow the SCSI subsystem scsi_execute_req() function to issue > requests using large buffers that are better allocated with vmalloc() > rather than kmalloc(), modify bio_map_kern() to allow passing a buffer > allocated with the vmalloc() function. To do so, simply test the buffer > address using is_vmalloc_addr() and use vmalloc_to_page() instead of > virt_to_page() to obtain the pages of vmalloc-ed buffers. This is broken on architectures with VIVT caches. You need to flush and invalidate the caches based on the virtual address on those before performing DMA operations.
On 2019/06/26 1:06, Chaitanya Kulkarni wrote: > On 06/25/2019 08:59 AM, Bart Van Assche wrote: >> On 6/24/19 8:24 PM, Chaitanya Kulkarni wrote: >>> nit:- Can we use is_vmalloc_addr() call directly so that >>> "if (is_vmalloc)" -> "if (is_vmalloc_addr(data))" and remove is_vmalloc >>> variable. >> That would change a single call of is_vmalloc_addr() into multiple? > > Well is_vmalloc_addr() it is an in-line helper with address comparison. > > is it too expensive to have such a comparison in the loop ? Probably not, but I do not see the point in calling it for every page either since the cost of the additional bool on the stack is likely also very low.
On 6/25/19 6:38 PM, Damien Le Moal wrote: > On 2019/06/26 1:06, Chaitanya Kulkarni wrote: >> On 06/25/2019 08:59 AM, Bart Van Assche wrote: >>> On 6/24/19 8:24 PM, Chaitanya Kulkarni wrote: >>>> nit:- Can we use is_vmalloc_addr() call directly so that >>>> "if (is_vmalloc)" -> "if (is_vmalloc_addr(data))" and remove is_vmalloc >>>> variable. >>> That would change a single call of is_vmalloc_addr() into multiple? >> >> Well is_vmalloc_addr() it is an in-line helper with address comparison. >> >> is it too expensive to have such a comparison in the loop ? > > Probably not, but I do not see the point in calling it for every page either > since the cost of the additional bool on the stack is likely also very low. > Okay.
diff --git a/block/bio.c b/block/bio.c index ce797d73bb43..05afcaf655f3 100644 --- a/block/bio.c +++ b/block/bio.c @@ -1501,6 +1501,8 @@ struct bio *bio_map_kern(struct request_queue *q, void *data, unsigned int len, unsigned long end = (kaddr + len + PAGE_SIZE - 1) >> PAGE_SHIFT; unsigned long start = kaddr >> PAGE_SHIFT; const int nr_pages = end - start; + bool is_vmalloc = is_vmalloc_addr(data); + struct page *page; int offset, i; struct bio *bio; @@ -1518,7 +1520,11 @@ struct bio *bio_map_kern(struct request_queue *q, void *data, unsigned int len, if (bytes > len) bytes = len; - if (bio_add_pc_page(q, bio, virt_to_page(data), bytes, + if (is_vmalloc) + page = vmalloc_to_page(data); + else + page = virt_to_page(data); + if (bio_add_pc_page(q, bio, page, bytes, offset) < bytes) { /* we don't support partial mappings */ bio_put(bio);
To allow the SCSI subsystem scsi_execute_req() function to issue requests using large buffers that are better allocated with vmalloc() rather than kmalloc(), modify bio_map_kern() to allow passing a buffer allocated with the vmalloc() function. To do so, simply test the buffer address using is_vmalloc_addr() and use vmalloc_to_page() instead of virt_to_page() to obtain the pages of vmalloc-ed buffers. Fixes: 515ce6061312 ("scsi: sd_zbc: Fix sd_zbc_report_zones() buffer allocation") Fixes: e76239a3748c ("block: add a report_zones method") Cc: stable@vger.kernel.org Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- block/bio.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)