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

["Qun-wei Lin (林群崴)" <Qun-wei.Lin@mediatek.com>]

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@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.
> >