Re: [Scst-devel] Subject: [PATCH] vdisk_fileio: Add support for zero-copy write

[Vladislav B. <vs...@vl...>]

Bart Van Assche, on 05/27/2012 09:59 AM wrote:
> On 05/25/12 20:32, Vladislav Bolkhovitin wrote:
>
>> mm/filemap.c:603 is:
>>
>> 	VM_BUG_ON(!PageLocked(page));
>>
>> Before that it worked well for quite some time with file on RAMFS.
>
>
> Strange. That means that write_end() was invoked on a page on which
> write_begin() hasn't been invoked. Does qla2x00t perhaps modify the sg
> list page pointers and/or size in some way ?

No, it doesn't. It only 1:1 maps them to DMA.

>> Also I have some questions:
>>
>> 1. You set for the file's mapping mapping_set_unevictable(). Won't it
>
>> prevent all pages of this file from be evicted from the page cache?
>> I.e. lock them there? Which, in case if file size>  RAM size
>> eventually must lead to OOM?
>
> As far as I know no explicit munlock() call is necessary - an implicit
> munlock() happens automatically when the page reference count drops to zero.

Yes, correct. So, why mapping_set_unevictable() is needed? Pages references should 
be sufficient.

>> 2. I'm not sure about overlapping WRITE commands. If they can lead to
>
>> a deadlock, won't it be dangerous, because any initiator can DoS the
>> target by simply bombing it by overlapped WRITE commands?
>
> The comment is wrong - the current approach should be fine. I had been
>
> experiencing deadlocks with a previous (unpublished) version of this
> patch in which I had allowed processing of another write command to
> start after write_begin() had already been invoked on the context of the
> same thread. The complete() / wait_for_completion() pair ensures that
> this cannot occur.

I see. This approach should work.

>> 3. Just in case, have you tested BIDI case? If not, you can easily do
>
>> it with sgv4_xdwriteread command. There's no real handler of this
>> command anywhere, but you will trigger the data transfer path anyway.
>
> Hadn't tested bidi yet since there is no bidi support in ib_srp. The
> attached patch should handle bidi correctly.

Thanks, I'll try it ASAP.