Message-ID: <c9cb1f3b-0b3b-c571-4a51-e647f3c1e90a@gmail.com>
Date:   Tue, 27 Jul 2021 02:54:15 +0200
From:   Bodo Stroesser <bostroesser@...il.com>
To:     Bart Van Assche <bvanassche@....org>,
        Christoph Hellwig <hch@....de>
Cc:     Joel Becker <jlbec@...lplan.org>, linux-kernel@...r.kernel.org,
        "Martin K . Petersen" <martin.petersen@...cle.com>,
        Yanko Kaneti <yaneti@...lera.com>,
        Brendan Higgins <brendanhiggins@...gle.com>
Subject: Re: [PATCH 2/4] configfs: Fix writing at a non-zero offset

On 26.07.21 23:52, Bart Van Assche wrote:
> On 7/26/21 2:13 PM, Bodo Stroesser wrote:
>> On 26.07.21 18:26, Bart Van Assche wrote:
>>> On 7/26/21 7:58 AM, Bodo Stroesser wrote:
>>>> On 23.07.21 23:23, Bart Van Assche wrote:
>>>> Let's say user writes 5 times to configfs file while keeping it open.
>>>> On every write() call it writes 1 character only, e.g. first "A", 
>>>> then "B", ...
>>>>
>>>> The original code before the changes 5 times called 
>>>> flush_write_buffer for the
>>>> strings "A\0", "B\0", ... (with the '\0' not included in the count 
>>>> parameter,
>>>> so count is 1 always, which is the length of the last write).
>>>
>>> Isn't that behavior a severe violation of how POSIX specifies that 
>>> the write() system call should be implemented?
>>
>> Hmm. I'm not sure which detail should violate POSIX spec? Is there any
>> definition how data should be flushed from buffer internally? (I'm by
>> far not a POSIX expert!)
>>
>> I would rather say the new behavior, to call flush_write_buffer during 
>> the
>> first write() for the data of that write, and then on the second write to
>> call flush_write_buffer for the concatenated data of the first and the
>> second write, could be a violation of POSIX, because the one times 
>> written
>> data of the first write is flushed twice.
>>
>> I don't like the idea of breaking the "one write, one flush" principle 
>> that
>> was implemented before. The old comment:
>> "There is no easy way for us to know if userspace is only doing a partial
>> write, so we don't support them. We expect the entire buffer to come 
>> on the
>> first write."
>> as I interpret it, makes clear that configfs code has to work 
>> according to
>> that principle. (Or even block all but the first write, but that would 
>> even
>> more break compatibility to old implementation.)
> 
> Hi Bodo,
> 
> The private email that you sent me made it clear that you would like to 
> keep the behavior from kernel 5.13. That means passing "A\0", "B\0", ... 
> to the configfs store callback function if "AB..." is witten one byte at 
> a time. What is not clear to me is how a store callback with argument 
> "B\0" can know at which offset that write happened? From 
> <linux/configfs.h> (I have added argument names):
> 
>      ssize_t (*store)(struct config_item *item, const char *page,
>                           size_t count);

It does not know. It simply handles it as two separate store actions.
One could say, both start from offset 0.

> 
> My understanding of the POSIX specification [1] is that writes should 
> happen at the proper offset. If user space software writes "A" at offset 
> 0 and "B" at offset 1 then the string "AB" should be passed to the 
> configfs store callback.

The comment says, that such a concatenation is not supported. To add
such a support, we would have to buffer all writes and then have a
criterion that triggers the flush_write_buffer. For example that could
be done on close(). But that would also mean, that bad result from store
handler could be reported by close only. And it would mean, that again
the behavior changes, in that the new SW allows one store action only
after one open(). You have to close and re-open before you can start a
new store action.

To me it looks strange to write again all previous data from the 
beginning at each new write. So I think this is not a good solution.

> 
> Regarding the "action" attribute from your tcmu patch, how about 
> checking the last character of the string written into that attribute 
> instead of the first character? Would that be sufficient to write twice 
> into that attribute without having to call close() and open() between 
> the two write actions?

I'm not sure I understand what you mean. If userspace writes a string
byte by byte or in pieces of other sizes, would you still gather
data in the file's buffer and call flush_write_buffer on each write
with all the data gathered up to and including the current write?

Of so, do you want the store handler to detect the end of the string,
e.g by searching for '\n', and discard the write if not found? That
would not work well, because after the store handler detected the '\n',
during the next write it would get the same string again plus what was
added by the new write. Store handler would have to know, how much of
the entire buffer content it already had seen. After a couple of writes
we would even run out of buffer. So again close and re-open is needed.
After close and re-open, how does the store handler know, that the
buffer now is re-started from the beginning?

The new behavior can also cause trouble with existing store handlers. 
Example:
The tcmu attribute files cmd_time_out and qfull_time_out just take a
string containing the decimal formatted number of seconds of the
timeout. Each number up to now had to be transferred in a single write.
Assume the old value is 30 and we want to change to 19. If userspace
writes byte by byte, you end up calling
store(item, "1\0", 1) and then
store(item, "19\9", 2).
If these quick changes do not cause trouble in tcmu's scsi cmd handling,
then think what happens, if userspace is interrupted between the two
writes. Allowing to split the writes cause a loss of "atomicity".

> 
> To me the following comment: "There is no easy way for us to know if 
> userspace is only doing a partial write, so we don't support them. We 
> expect the entire buffer to come on the first write." means that writing 
> "ABCD" by first writing "AB" and next "CD" will trigger two 
> item->store() calls. Triggering a single item->store() call for partial 
> writes is not supported.

Exactly. So IMHO we don't need to handle any offsets during write
processing, since for every write we again start at offset 0.
(We just add the trailing '\0' - not in count - to ease store handler's
work.)

Thank you,
Bodo

Powered by blists - more mailing lists