Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 2/4] configfs: Fix writing at a non-zero offset
To:     Bart Van Assche <bvanassche@acm.org>,
        Christoph Hellwig <hch@lst.de>
Cc:     Joel Becker <jlbec@evilplan.org>, linux-kernel@vger.kernel.org,
        "Martin K . Petersen" <martin.petersen@oracle.com>,
        Yanko Kaneti <yaneti@declera.com>,
        Brendan Higgins <brendanhiggins@google.com>
References: <20210723212353.896343-1-bvanassche@acm.org>
 <20210723212353.896343-3-bvanassche@acm.org>
 <7bee65ce-f5f1-a525-c72d-221b5d23cf3e@gmail.com>
 <d12f24b6-7066-f9bb-1b88-6cc23c9c45c1@acm.org>
 <4055ca70-7669-d00d-7c08-86fe75a3d377@gmail.com>
 <618b2bdc-282b-0a1d-1fc5-020cf80d7a7e@acm.org>
 <c9cb1f3b-0b3b-c571-4a51-e647f3c1e90a@gmail.com>
 <ab190c50-8c87-b215-1432-056c81bcd656@acm.org>
From:   Bodo Stroesser <bostroesser@gmail.com>
Message-ID: <fec30933-46b1-1085-1af1-1fd0d2265981@gmail.com>
Date:   Tue, 27 Jul 2021 09:27:48 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <ab190c50-8c87-b215-1432-056c81bcd656@acm.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 27.07.21 05:17, Bart Van Assche wrote:
> On 7/26/21 5:54 PM, Bodo Stroesser wrote:
>> 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".
> 
>  From Documentation/filesystems/configfs.rst, for normal attributes:
> "Configfs expects write(2) to store the entire buffer at once." In other
> words, the behavior for partial writes is undocumented. My changes
> preserve the behavior if a buffer is written in its entirety. I do not
> agree that my changes can cause trouble for existing store handlers.
> 

I agree. I was not precise.

What I meant is, that changing the source code in such a way, that
writing a buffer in multiple writes works in general, could cause
trouble in case userspace uses this.

But for special syscall sequences your changes still change the result
on existing configfs files. Example:

1) userspace program opens qfull_time_out
2) userspace program writes "90", count=2 to set timeout to 90 sec
3) userspace again wants to change timeout, so it writes "55", count=2

Before the changes we end up with timeout being 55 seconds. After the
change - due to data gathering - we finally have timeout 9055 seconds.

BR,
Bodo