Re: [PATCH 0/2] Add BLK_FEAT_READ_SYNCHRONOUS and SWP_READ_SYNCHRONOUS_IO

[Chris Li <chrisl@kernel.org>]

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@infradead.org> wrote:
>
> Well, you're not actually setting your new flags anywhere, which -
> as you might know - is an reson for an insta-NAK.
>