Re: [Iscsitarget-devel] [PATCH] WRITE SAME 16 fixes

[Shivaram U. <shi...@qu...>]

On Wed, Jul 3, 2013 at 1:25 AM, Shivaram Upadhyayula <
shi...@qu...> wrote:

> ... (removed)
>


>         while (tio_index < tio->pg_cnt) {
> .... (removed)
>           In the above loop, we add the 512 bytes to the bio in the first
> iteration itself. In the second iteration of the above loops bytes == size
> == 0 and so on till 256 and in the second iteration we call bio_add_page
> with size 0.
>
> In the file-io case, as you mention its more an optimization. We end up
> calling vfs_write() with zero count.
>
> Thinking of a simpler check, Wouldn't replacing while/for (tio_index <
tio->pg_cnt) with while (size) {} work ?
Regards,
Shivaram




> Regards,
> Shivaram
>
>
>
> On Wed, Jul 3, 2013 at 1:06 AM, Arne Redlich <arn...@go...>wrote:
>
>> Shivaram,
>>
>> [sorry for sending it again - forgot to CC iet-devel last time around]
>>
>> 2013/6/30 Shivaram Upadhyayula <shi...@qu...>:
>> > There are many issues with the current implementation
>> > 1. target_tio has an extra tio_get() which prevents it from getting
>> freed
>> > 2. length and media_length calculations need to be 64 bit
>> > 3. MAX_IO_SIZE is actually set to 1GB rather than 1MB
>> > 4. If the length is greater than 1MB say 1MB + 512 then in the last
>> > iteration we need to write 512 bytes but then tio pg_cnt is greater
>> than 1,
>> > so we end up with a bio_add_page() with size 0
>> >
>> > Patch attached
>>
>> Thanks for your patch. I took the liberty of splitting it, and for now
>> I only applied the fixes for (1) - (3). Please verify I didn't break
>> anything inadvertently.
>>
>> The fix for (4) (reproduced below for those interested) needs to go
>> into a different patch as it's not directly related to WRITE SAME.
>> Also, I'm at the moment not fully clear whether it's an actual bug -
>> did you run into issues / can you point out the problem for blockio?
>> The fileio change does not look like a bug but rather like an
>> optimization, right?
>>
>> Thanks,
>> Arne
>>
>> diff --git a/kernel/block-io.c b/kernel/block-io.c
>> index dffc4d0..7c9ea21 100644
>> --- a/kernel/block-io.c
>> +++ b/kernel/block-io.c
>> @@ -79,7 +79,7 @@ blockio_make_request(struct iet_volume *volume,
>> struct tio *tio, int rw)
>>         init_completion(&tio_work->tio_complete);
>>
>>         /* Main processing loop, allocate and fill all bios */
>> -       while (tio_index < tio->pg_cnt) {
>> +       while (size && tio_index < tio->pg_cnt) {
>>                 bio = bio_alloc(GFP_KERNEL, min(max_pages,
>> BIO_MAX_PAGES));
>>                 if (!bio) {
>>                         err = -ENOMEM;
>> @@ -100,7 +100,7 @@ blockio_make_request(struct iet_volume *volume,
>> struct tio *tio, int rw)
>>                 atomic_inc(&tio_work->bios_remaining);
>>
>>                 /* Loop for filling bio */
>> -               while (tio_index < tio->pg_cnt) {
>> +               while (size && tio_index < tio->pg_cnt) {
>>                         unsigned int bytes = PAGE_SIZE;
>>
>>                         if (bytes > size)
>> diff --git a/kernel/file-io.c b/kernel/file-io.c
>> index 21a986a..8c4a7f7 100644
>> --- a/kernel/file-io.c
>> +++ b/kernel/file-io.c
>> @@ -34,7 +34,7 @@ static int fileio_make_request(struct iet_volume
>> *lu, struct tio *tio, int rw)
>>         size = tio->size;
>>         ppos = tio->offset;
>>
>> -       for (i = 0; i < tio->pg_cnt; i++) {
>> +       for (i = 0; i < tio->pg_cnt && size; i++) {
>>                 page = tio->pvec[i];
>>                 assert(page);
>>                 buf = page_address(page);
>>
>
>
>
> --
> QUADStor Open Source Storage Virtualization : Thin Provisioning, Data
> Deduplication, VAAI, High Availability
> http://www.quadstor.com
>



-- 
QUADStor Open Source Storage Virtualization : Thin Provisioning, Data
Deduplication, VAAI, High Availability
http://www.quadstor.com