netdev - Re: [PATCH AUTOSEL 4.19 140/219] libceph: drop last_piece logic from write_partial_message

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20191201143436.GS5861@sasha-vm>
Date:   Sun, 1 Dec 2019 09:34:36 -0500
From:   Sasha Levin <sashal@...nel.org>
To:     Ilya Dryomov <idryomov@...il.com>
Cc:     LKML <linux-kernel@...r.kernel.org>, stable@...r.kernel.org,
        Ceph Development <ceph-devel@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH AUTOSEL 4.19 140/219] libceph: drop last_piece logic from
 write_partial_message_data()

On Fri, Nov 22, 2019 at 03:00:43PM +0100, Ilya Dryomov wrote:
>On Fri, Nov 22, 2019 at 6:51 AM Sasha Levin <sashal@...nel.org> wrote:
>>
>> From: Ilya Dryomov <idryomov@...il.com>
>>
>> [ Upstream commit 1f6b821aef78e3d79e8d598ae59fc7e23fb6c563 ]
>>
>> last_piece is for the last piece in the current data item, not in the
>> entire data payload of the message.  This is harmful for messages with
>> multiple data items.  On top of that, we don't need to signal the end
>> of a data payload either because it is always followed by a footer.
>>
>> We used to signal "more" unconditionally, until commit fe38a2b67bc6
>> ("libceph: start defining message data cursor").  Part of a large
>> series, it introduced cursor->last_piece and also mistakenly inverted
>> the hint by passing last_piece for "more".  This was corrected with
>> commit c2cfa1940097 ("libceph: Fix ceph_tcp_sendpage()'s more boolean
>> usage").
>>
>> As it is, last_piece is not helping at all: because Nagle algorithm is
>> disabled, for a simple message with two 512-byte data items we end up
>> emitting three packets: front + first data item, second data item and
>> footer.  Go back to the original pre-fe38a2b67bc6 behavior -- a single
>> packet in most cases.
>>
>> Signed-off-by: Ilya Dryomov <idryomov@...il.com>
>> Signed-off-by: Sasha Levin <sashal@...nel.org>
>> ---
>>  net/ceph/messenger.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
>> index f7d7f32ac673c..6514816947fbe 100644
>> --- a/net/ceph/messenger.c
>> +++ b/net/ceph/messenger.c
>> @@ -1612,7 +1612,6 @@ static int write_partial_message_data(struct ceph_connection *con)
>>                 struct page *page;
>>                 size_t page_offset;
>>                 size_t length;
>> -               bool last_piece;
>>                 int ret;
>>
>>                 if (!cursor->resid) {
>> @@ -1620,10 +1619,9 @@ static int write_partial_message_data(struct ceph_connection *con)
>>                         continue;
>>                 }
>>
>> -               page = ceph_msg_data_next(cursor, &page_offset, &length,
>> -                                         &last_piece);
>> -               ret = ceph_tcp_sendpage(con->sock, page, page_offset,
>> -                                       length, !last_piece);
>> +               page = ceph_msg_data_next(cursor, &page_offset, &length, NULL);
>> +               ret = ceph_tcp_sendpage(con->sock, page, page_offset, length,
>> +                                       true);
>>                 if (ret <= 0) {
>>                         if (do_datacrc)
>>                                 msg->footer.data_crc = cpu_to_le32(crc);
>
>Hi Sasha,
>
>This commit was part of a larger series and shouldn't be backported on
>its own.  Please drop it.

Now dropped, thanks!

-- 
Thanks,
Sasha