On 6/27/19 11:09 PM, Christoph Hellwig wrote:
>> @@ -79,6 +80,7 @@ static int sd_zbc_do_report_zones(struct scsi_disk *sdkp, unsigned char *buf,
>> put_unaligned_be32(buflen, &cmd[10]);
>> if (partial)
>> cmd[14] = ZBC_REPORT_ZONE_PARTIAL;
>> +
>> memset(buf, 0, buflen);
>>
>> result = scsi_execute_req(sdp, cmd, DMA_FROM_DEVICE,
>
> Spurious whitespace change.
Fixed.
>> +static void *sd_zbc_alloc_report_buffer(struct request_queue *q,
>> + unsigned int nr_zones, size_t *buflen,
>> + gfp_t gfp_mask)
>> +{
>> + size_t bufsize;
>> + void *buf;
>> +
>> + /*
>> + * Report zone buffer size should be at most 64B times the number of
>> + * zones requested plus the 64B reply header, but should be at least
>> + * SECTOR_SIZE for ATA devices.
>> + * Make sure that this size does not exceed the hardware capabilities.
>> + * Furthermore, since the report zone command cannot be split, make
>> + * sure that the allocated buffer can always be mapped by limiting the
>> + * number of pages allocated to the HBA max segments limit.
>> + */
>> + nr_zones = min(nr_zones, SD_ZBC_REPORT_MAX_ZONES);
>> + bufsize = roundup((nr_zones + 1) * 64, 512);
>> + bufsize = min_t(size_t, bufsize,
>> + queue_max_hw_sectors(q) << SECTOR_SHIFT);
>> + bufsize = min_t(size_t, bufsize, queue_max_segments(q) << PAGE_SHIFT);
>> +
>> + buf = __vmalloc(bufsize, gfp_mask, PAGE_KERNEL);
>
> __vmalloc is odd in that it takes a gfp parameter, but can't actually
> use it for the page table allocations. So you'll need to do
> memalloc_noio_save here, or even better do that in the block layer and
> remove the gfp_t parameter from ->report_zones.
Yes, indeed. However, removing the gfp_flags from report_zones method
would limit possibilities to only GFP_NOIO or GFP_KERNEL (default
vmalloc). What if the caller is an FS and needs GFP_NOFS, or any other
reclaim flags ? Handling all possibilities does not seem reasonable.
Handling only GFP_KERNEL and GFP_IO is easy, but that would mean that
the caller of blkdev_report_zones would need to do itself calls to
whatever memalloc_noXX_save/restore() it needs. Is that OK ?
Currently, blkdev_report_zones() uses only either GFP_KERNEL (general
case, fs, dm and user ioctl), or GFP_NOIO for revalidate, disk scan and
dm-zoned error path. So removing the flag from the report zones method
while keeping it in the block layer API to distinguished these cases is
simple, but I am not sure if that will not pause problems for some
users. Thoughts ?
--
Damien Le Moal
Western Digital Research
