Date:   Sat, 3 Oct 2020 18:42:45 +0800
From:   Coly Li <colyli@...e.de>
To:     David Miller <davem@...emloft.net>
Cc:     linux-block@...r.kernel.org, linux-nvme@...ts.infradead.org,
        netdev@...r.kernel.org, open-iscsi@...glegroups.com,
        linux-scsi@...r.kernel.org, ceph-devel@...r.kernel.org,
        linux-kernel@...r.kernel.org, chaitanya.kulkarni@....com,
        cleech@...hat.com, hch@....de, amwang@...hat.com,
        eric.dumazet@...il.com, hare@...e.de, idryomov@...il.com,
        jack@...e.com, jlayton@...nel.org, axboe@...nel.dk,
        lduncan@...e.com, michaelc@...wisc.edu,
        mskorzhinskiy@...arflare.com, philipp.reisner@...bit.com,
        sagi@...mberg.me, vvs@...tuozzo.com, vbabka@...e.com
Subject: ...

On 2020/10/3 06:28, David Miller wrote:
> From: Coly Li <colyli@...e.de>
> Date: Fri,  2 Oct 2020 16:27:27 +0800
> 
>> As Sagi Grimberg suggested, the original fix is refind to a more common
>> inline routine:
>>     static inline bool sendpage_ok(struct page *page)
>>     {
>>         return  (!PageSlab(page) && page_count(page) >= 1);
>>     }
>> If sendpage_ok() returns true, the checking page can be handled by the
>> concrete zero-copy sendpage method in network layer.
> 
> Series applied.
> 
>> The v10 series has 7 patches, fixes a WARN_ONCE() usage from v9 series,
>  ...
> 
> I still haven't heard from you how such a fundamental build failure
> was even possible.
> 

Hi David,

Here is the detail steps how I leaked this uncompleted patch to you,
1) Add WARN_ONCE() as WARN_ON() to kernel_sendpage(). Maybe I was still
hesitating when I typed WARN_ONCE() on keyboard.
2) Generate the patches, prepare to post
3) Hmm, compiling failed, oh it is WARN_ONCE(). Yeah, WARN_ONCE() might
be more informative and better.
4) Modify to use WARN_ONCE() and compile and try, looks fine.
5) Re-generate the patches to overwrite the previous ones.
6) Post the patches.

The missing part was, before I post the patches, I should do rebase and
commit the change, but (interrupted by other stuffs) it skipped in my
mind. Although I regenerated the series but the change was not included.
The result was, uncompleted patch posted and the second-half change
still stayed in my local file.


> If the v9 patch series did not even compile, how in the world did you
> perform functional testing of these changes?
> 

Only 0002-net-add-WARN_ONCE-in-kernel_sendpage-for-improper-ze.patch was
tested in v9 series, other tests were done in previous versions.

> Please explain this to me, instead of just quietly fixing it and
> posting an updated series.


And not all the patches in the series were tested. Here is the testing
coverage of the series:

The following ones were tested and verified to break nothing and avoid
the mm corruption and panic,
0001-net-introduce-helper-sendpage_ok-in-include-linux-ne.patch
0002-net-add-WARN_ONCE-in-kernel_sendpage-for-improper-ze.patch
0003-nvme-tcp-check-page-by-sendpage_ok-before-calling-ke.patch
0006-scsi-libiscsi-use-sendpage_ok-in-iscsi_tcp_segment_m.patch

The following ones were not tested, due to complicated environment setup,
0005-drbd-code-cleanup-by-using-sendpage_ok-to-check-page.patch
0007-libceph-use-sendpage_ok-in-ceph_tcp_sendpage.patch

This patch I didn't explicitly test, due to lack of knowledge to modify
network code to trigger a buggy condition. It just went with other
tested patches,
0004-tcp-use-sendpage_ok-to-detect-misused-.sendpage.patch


Back to the built failure, I don't have excuse for leaking this
uncompleted version to you. Of cause I will try to avoid to
inefficiently occupy maintainer's time by such silly mess up.

Thanks for your review and the thorough maintenance.

Coly Li

Powered by blists - more mailing lists