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: <120a91c2b71256d7723c6de7d98c611899b84bac.camel@mediatek.com>
Date: Fri, 27 Sep 2024 10:14:19 +0000
From: Qun-wei Lin (林群崴) <Qun-wei.Lin@...iatek.com>
To: "hch@...radead.org" <hch@...radead.org>, "chrisl@...nel.org"
	<chrisl@...nel.org>
CC: Andrew Yang (楊智強) <Andrew.Yang@...iatek.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"baohua@...nel.org" <baohua@...nel.org>, "linux-block@...r.kernel.org"
	<linux-block@...r.kernel.org>, "linux-mediatek@...ts.infradead.org"
	<linux-mediatek@...ts.infradead.org>, "willy@...radead.org"
	<willy@...radead.org>, "linux-mm@...ck.org" <linux-mm@...ck.org>,
	wsd_upstream <wsd_upstream@...iatek.com>, "david@...hat.com"
	<david@...hat.com>, "schatzberg.dan@...il.com" <schatzberg.dan@...il.com>,
	Chinwen Chang (張錦文)
	<chinwen.chang@...iatek.com>, John Hsu (許永翰)
	<John.Hsu@...iatek.com>, "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
	"axboe@...nel.dk" <axboe@...nel.dk>, "akpm@...ux-foundation.org"
	<akpm@...ux-foundation.org>, "kasong@...cent.com" <kasong@...cent.com>,
	Casper Li (李中榮) <casper.li@...iatek.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "matthias.bgg@...il.com"
	<matthias.bgg@...il.com>, "ying.huang@...el.com" <ying.huang@...el.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	"ryan.roberts@....com" <ryan.roberts@....com>
Subject: Re: [PATCH 0/2] Add BLK_FEAT_READ_SYNCHRONOUS and
 SWP_READ_SYNCHRONOUS_IO

Hi Chris & Christoph,

Thank you for your review and valuable feedback.
 
I will submit a v2 version of the patch.
In this version, I will ensure that 'bdev_syncronous()' sets both
'SWP_READ_SYNCHRONOUS_IO' and 'SWP_WRITE_SYNCHRONOUS_IO' flags.

Best regards,

Qun-Wei


On Fri, 2024-09-20 at 17:04 -0700, Chris Li wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  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.
> >