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