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.

Patch 1:
- introduce BLK_FEAT_READ_SYNCHRONOUS

Patch 2:
- introduce SWP_READ_SYNCHRONOUS_IO

Qun-Wei Lin (2):
  block: add BLK_FEAT_READ_SYNCHRONOUS feature for synchronous read
  mm, swap: introduce SWP_READ_SYNCHRONOUS_IO

 include/linux/blkdev.h |  8 ++++++++
 include/linux/swap.h   | 31 ++++++++++++++++---------------
 mm/memory.c            |  3 ++-
 mm/page_io.c           |  2 +-
 mm/swapfile.c          |  3 +++
 5 files changed, 30 insertions(+), 17 deletions(-)

-- 
2.45.2

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

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

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