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