Re: [PATCH V4 07/16] scsi: sd_zbc: Initialize device request queue zoned data

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 9/24/17 17:07, Christoph Hellwig wrote:
>> +static inline unsigned long *sd_zbc_alloc_zone_bitmap(struct scsi_disk *sdkp)
>> +{
>> +	struct request_queue *q = sdkp->disk->queue;
>> +
>> +	return kzalloc_node(BITS_TO_LONGS(sdkp->nr_zones)
>> +			    * sizeof(unsigned long),
>> +			    GFP_KERNEL, q->node);
> 
> This really screams for kcalloc_node and friends that I think Johannes
> volunteered to add.

OK. Should I wait for Johannes patches ? That can be easily changed
later though.

>> + * sd_zbc_setup_seq_zones - Initialize the disk request queue zone type bitmap.
>> + * @sdkp: The disk of the bitmap
>> + *
>> + * Allocate a zone bitmap and initialize it by identifying sequential zones.
>> + */
>> +static int sd_zbc_setup_seq_zones(struct scsi_disk *sdkp)
>> +{
>> +	struct request_queue *q = sdkp->disk->queue;
>> +	unsigned long *seq_zones;
>> +	sector_t block = 0;
>> +	unsigned char *buf;
>> +	unsigned char *rec;
>> +	unsigned int buf_len;
>> +	unsigned int list_length;
>> +	unsigned int n = 0;
>> +	u8 type, cond;
>> +	int ret = -ENOMEM;
>> +
>> +	kfree(q->seq_zones);
>> +	q->seq_zones = NULL;
> 
> We also free the previous version, which isn't documented above.
> Which in general begs the question:  What scheme protects access
> to q->seq_zones?

Yes, indeed, the comments do not mention that. I will add that.
As for the protection, there is none necessary I think. The reason is
that the previous version free+alloc can only happen if sd_revalidate is
called, at which point there are no write commands on-going, so no
references to seq_zones. Is this correct/not correct ?

I am not even sure if the reallocation/reinit is even necessary though.
Since sd_revalidate() will at worst result in the disk capacity going to
0 (if the disk is non responsive for instance), accesses beyond
seq_zones size will never happen. And the zone types never change, so it
may be better to drop this reallocation+reinit. Same for the zones_wlock
bitmap. What do you hink ?

> And the previous patch should probably grow a comment to document
> that q->seq_zones is entirely managed by the driver.

Will do.

>> +		/*
>> +		 * Parse reported zone descriptors to find sequiential zones.
>> +		 * Since read-only and offline zones cannot be written, do not
>> +		 * mark them as sequential in the bitmap.
>> +		 */
>> +		list_length = get_unaligned_be32(&buf[0]) + 64;
>> +		rec = buf + 64;
>> +		buf_len = min(list_length, SD_ZBC_BUF_SIZE);
>> +		while (rec < buf + buf_len) {
>> +			type = rec[0] & 0x0f;
>> +			cond = (rec[1] >> 4) & 0xf;
>> +			if (type != ZBC_ZONE_TYPE_CONV &&
>> +			    cond != ZBC_ZONE_COND_READONLY &&
>> +			    cond != ZBC_ZONE_COND_OFFLINE)
>> +				set_bit(n, seq_zones);
>> +			block = get_unaligned_be64(&rec[8]) +
>> +				get_unaligned_be64(&rec[16]);
>> +			rec += 64;
>> +			n++;
>> +		}
> 
> Split this out into a helper?

Yes, that would be a nice cleanup. Will do.

Thanks.


-- 
Damien Le Moal
Western Digital Research



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux