linux-kernel - Re: [PATCH 0/2] Add BLK_FEAT_READ_SYNCHRONOUS and SWP_READ_SYNCHRONOUS

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACePvbUCNxy3sf6+7hk9HPGTNtTMbA2=Entu0xbV7TbwX4M2WQ@mail.gmail.com>
Date: Fri, 20 Sep 2024 17:04:51 -0700
From: Chris Li <chrisl@...nel.org>
To: Christoph Hellwig <hch@...radead.org>
Cc: Qun-Wei Lin <qun-wei.lin@...iatek.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	Matthias Brugger <matthias.bgg@...il.com>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, 
	Ryan Roberts <ryan.roberts@....com>, "Huang, Ying" <ying.huang@...el.com>, 
	David Hildenbrand <david@...hat.com>, "Matthew Wilcox (Oracle)" <willy@...radead.org>, 
	Al Viro <viro@...iv.linux.org.uk>, Dan Schatzberg <schatzberg.dan@...il.com>, 
	Kairui Song <kasong@...cent.com>, Barry Song <baohua@...nel.org>, Jens Axboe <axboe@...nel.dk>, 
	linux-kernel@...r.kernel.org, linux-mm@...ck.org, 
	linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org, 
	linux-block@...r.kernel.org, Casper Li <casper.li@...iatek.com>, 
	Chinwen Chang <chinwen.chang@...iatek.com>, Andrew Yang <andrew.yang@...iatek.com>, 
	John Hsu <john.hsu@...iatek.com>, wsd_upstream@...iatek.com
Subject: Re: [PATCH 0/2] Add BLK_FEAT_READ_SYNCHRONOUS and SWP_READ_SYNCHRONOUS_IO

Hi Qun-Wei,

Agree with Christoph that BLK_FEAT_READ_SYNCHRONOUS is not set
anywhere. That needs to be fixed.

Having a flag for BLK_FEAT_READ_SYNCHRONOUS and another flag for
BLK_FEAT_SYNCHRONOUS is just confusing.
for example, read path need to test two bits: "sis->flags &
(SWP_SYNCHRONOUS_IO | SWP_READ_SYNCHRONOUS_IO)"

There is only one caller of the bdev_synchronous(), which is in swapfile.c.

I suggest if you have  BLK_FEAT_READ_SYNCHRONOUS, you should have a
BLK_FEAT_WRITE_SYNCHRONOUS for writing.
The previous path that test the SWP_SYNCHRONOUS_IO should convert into
one of tests of SWP_READ_SYNCHRONOUS_IO or  SWP_WRITE_SYNCHRONOUS_IO
depend on the read or write path (never both).

"sis->flags & (SWP_SYNCHRONOUS_IO | SWP_READ_SYNCHRONOUS_IO)" will
change into "sis->flags & SWP_READ_SYNCHRONOUS_IO"

Then you can have  bdev_synchronous() just return the
SWP_READ_SYNCHRONOUS_IO | SWP_WRITE_SYNCHRONOUS_IO if both are set.
You don't need to have just bdev_synchronous() and
bdev_read_synchronous(). That is more boilerplate code which is
unnecessary.

I also suggest you squish your two patches into one because there is
no user of bdev_read_synchronous() in the first patch.
You should introduce the function with the code that uses it. Yes,
yes, I know you want to have a seperate patch for define vs another
patch for using it. In this case there is no good reason for that.

Best regards,

Chris

On Thu, Sep 19, 2024 at 4:37 AM Christoph Hellwig <hch@...radead.org> wrote:
>
> Well, you're not actually setting your new flags anywhere, which -
> as you might know - is an reson for an insta-NAK.
>