linux-kernel - Re: [PATCH 3/3] target/iblock: Add WRITE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 15 Nov 2012 10:03:26 -0500
From:	Douglas Gilbert <dgilbert@...erlog.com>
To:	Christoph Hellwig <hch@...radead.org>
CC:	"Nicholas A. Bellinger" <nab@...ux-iscsi.org>,
	target-devel <target-devel@...r.kernel.org>,
	linux-scsi <linux-scsi@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Christoph Hellwig <hch@....de>,
	"Martin K. Petersen" <martin.petersen@...cle.com>
Subject: Re: [PATCH 3/3] target/iblock: Add WRITE_SAME w/ UNMAP=0 emulation
 support

On 12-11-15 06:04 AM, Christoph Hellwig wrote:
>> +	/*
>> +	 * Enable WRITE_SAME emulation for IBLOCK, use scsi_debug.c default
>> +	 */
>
> Why would we care what scsi_debug.c uses?

Would you prefer no hint of where the magic number came
from? At least somebody who cares when they see that comment
might contact Martin Petersen and ask why he chose that
value.

t10.org documents are not big on sensible default values;
my favourite is where "0" means vendor specific.

Doug Gilbert

>> +	dev->dev_attrib.max_write_same_len = 0xFFFF;
>>
>>   	if (blk_queue_nonrot(q))
>>   		dev->dev_attrib.is_nonrot = 1;
>> @@ -375,22 +379,80 @@ err:
>>   	return ret;
>>   }
>
>> +static struct bio *iblock_get_bio(struct se_cmd *, sector_t, u32);
>> +static void iblock_submit_bios(struct bio_list *, int);
>> +static void iblock_complete_cmd(struct se_cmd *);
>
> I'd suggest moving the write_same callback below these to avoid
> forward declarations.
>
>> +	if (cmd->se_cmd_flags & SCF_WRITE_SAME_DISCARD) {
>
> I'd probably add separate write_same and write_same_unmap members to
> the sbc_ops structure.  That'll keep decoding which one is used in the
> SBC code, and it'll keep the implementations nicely separated.
>
>> +	if (sectors > cmd->se_dev->dev_attrib.max_write_same_len) {
>
> This sort of check should stay in the SBC code.
>
>> +	sg = &cmd->t_data_sg[0];
>
> Btw, it seems like we don't bother to ensure the S/G list length
> is just one sector for WRITE SAME with either the unmap bit set or not.
>
> Also please add testcases for WRITE SAME including corner cases like
> incorrect transfer length to the scsi testsuite to ensure this code
> has proper QA coverage.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/