archive mirror
 help / color / mirror / Atom feed
From: Bodo Stroesser <>
To: Bart Van Assche <>, Christoph Hellwig <>
Cc: Joel Becker <>,,
	"Martin K . Petersen" <>,
	Yanko Kaneti <>,
	Brendan Higgins <>
Subject: Re: [PATCH 2/4] configfs: Fix writing at a non-zero offset
Date: Tue, 27 Jul 2021 09:27:48 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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.


  reply	other threads:[~2021-07-27  7:27 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-23 21:23 [PATCH 0/4] Improve the configfs read and write iterators further Bart Van Assche
2021-07-23 21:23 ` [PATCH 1/4] configfs: Rework the overflow check in fill_write_buffer() Bart Van Assche
2021-07-23 21:23 ` [PATCH 2/4] configfs: Fix writing at a non-zero offset Bart Van Assche
2021-07-26 14:58   ` Bodo Stroesser
2021-07-26 16:26     ` Bart Van Assche
2021-07-26 21:13       ` Bodo Stroesser
2021-07-26 21:52         ` Bart Van Assche
2021-07-27  0:54           ` Bodo Stroesser
2021-07-27  3:17             ` Bart Van Assche
2021-07-27  7:27               ` Bodo Stroesser [this message]
2021-07-27 16:47                 ` Bart Van Assche
2021-07-28 17:14                   ` Bodo Stroesser
2021-07-28 17:55                     ` Bart Van Assche
2021-07-23 21:23 ` [PATCH 3/4] kunit: Add support for suite initialization and cleanup Bart Van Assche
2021-07-27 21:26   ` Brendan Higgins
2021-07-29  3:33     ` Bart Van Assche
2021-07-23 21:23 ` [PATCH 4/4] configfs: Add unit tests Bart Van Assche

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).