| Message ID | 20240919112952.981-1-qun-wei.lin@mediatek.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add BLK_FEAT_READ_SYNCHRONOUS and SWP_READ_SYNCHRONOUS_IO | expand |
Well, you're not actually setting your new flags anywhere, which - as you might know - is an reson for an insta-NAK.
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. >
Qun-Wei Lin <qun-wei.lin@mediatek.com> writes: > This patchset introduces 2 new feature flags, BLK_FEAT_READ_SYNCHRONOUS and > SWP_READ_SYNCHRONOUS_IO. > > These changes are motivated by the need to better accommodate certain swap > devices that support synchronous read operations but asynchronous write > operations. > > The existing BLK_FEAT_SYNCHRONOUS and SWP_SYNCHRONOUS_IO flags are not > sufficient for these devices, as they enforce synchronous behavior for both > read and write operations. Which kind of device needs this? Read fast, but write slow? -- Best Regards, Huang, Ying
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. > >
Hi Ying, Thank you for your question. The primary motivation for these new feature flags is to handle scenarios where we want read operations to be completed within the submit context, while write operations are handled in a different context. This does not necessarily imply that the write operations are slow; rather, it is about optimizing the handling of read and write operations based on their specific characteristics and requirements. Best Regards, Qun-Wei On Wed, 2024-09-25 at 15:34 +0800, Huang, Ying wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Qun-Wei Lin <qun-wei.lin@mediatek.com> writes: > > > This patchset introduces 2 new feature flags, > BLK_FEAT_READ_SYNCHRONOUS and > > SWP_READ_SYNCHRONOUS_IO. > > > > These changes are motivated by the need to better accommodate > certain swap > > devices that support synchronous read operations but asynchronous > write > > operations. > > > > The existing BLK_FEAT_SYNCHRONOUS and SWP_SYNCHRONOUS_IO flags are > not > > sufficient for these devices, as they enforce synchronous behavior > for both > > read and write operations. > > Which kind of device needs this? Read fast, but write slow? > > -- > Best Regards, > Huang, Ying
On Fri, Oct 11, 2024 at 09:08:10AM +0000, Qun-wei Lin (林群崴) wrote: > The primary motivation for these new feature flags is to handle > scenarios where we want read operations to be completed within the > submit context, while write operations are handled in a different > context. > > This does not necessarily imply that the write operations are slow; > rather, it is about optimizing the handling of read and write > operations based on their specific characteristics and requirements. So why wouldn't we always want to do that instead of making it a per-bdev flag?