[PATCH 0/2] Add BLK_FEAT_READ_SYNCHRONOUS and SWP_READ_SYNCHRONOUS_IO

[PATCH 0/2] Add BLK_FEAT_READ_SYNCHRONOUS and SWP_READ_SYNCHRONOUS_IO

[Qun-Wei Lin]

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

[PATCH 1/2] block: add BLK_FEAT_READ_SYNCHRONOUS feature for synchronous read

[Qun-Wei Lin]

This commit introduces a new feature flag, BLK_FEAT_READ_SYNCHRONOUS,
which ensures that read operations always complete in the submit context.
This is useful for scenarios where synchronous read operations are required
while allowing write operations to remain asynchronous.

This patch is preparation for optimizing of the operation of the swap
device with next patch.

Signed-off-by: Qun-Wei Lin <qun-wei.lin@mediatek.com>
---
 include/linux/blkdev.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 643c9020a35a..a697db11d91c 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -332,6 +332,9 @@ typedef unsigned int __bitwise blk_features_t;
 #define BLK_FEAT_RAID_PARTIAL_STRIPES_EXPENSIVE \
 	((__force blk_features_t)(1u << 15))
 
+/* read operations always completes in submit context */
+#define BLK_FEAT_READ_SYNCHRONOUS	((__force blk_features_t)(1u << 16))
+
 /*
  * Flags automatically inherited when stacking limits.
  */
@@ -1310,6 +1313,11 @@ static inline bool bdev_synchronous(struct block_device *bdev)
 	return bdev->bd_disk->queue->limits.features & BLK_FEAT_SYNCHRONOUS;
 }
 
+static inline bool bdev_read_synchronous(struct block_device *bdev)
+{
+	return bdev->bd_disk->queue->limits.features & BLK_FEAT_READ_SYNCHRONOUS;
+}
+
 static inline bool bdev_stable_writes(struct block_device *bdev)
 {
 	struct request_queue *q = bdev_get_queue(bdev);
-- 
2.45.2

[PATCH 2/2] mm, swap: introduce SWP_READ_SYNCHRONOUS_IO

[Qun-Wei Lin]

The existing SWP_SYNCHRONOUS_IO flag is not enough for certain swap
devices that support synchronous read operations but asynchronous write
operations, so we need to introduce a new flag SWP_READ_SYNCHRONOUS_IO.

Signed-off-by: Qun-Wei Lin <qun-wei.lin@mediatek.com>
---
 include/linux/swap.h | 31 ++++++++++++++++---------------
 mm/memory.c          |  3 ++-
 mm/page_io.c         |  2 +-
 mm/swapfile.c        |  3 +++
 4 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/include/linux/swap.h b/include/linux/swap.h
index ba7ea95d1c57..f595050f431b 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -205,21 +205,22 @@ struct swap_extent {
 	  offsetof(union swap_header, info.badpages)) / sizeof(int))
 
 enum {
-	SWP_USED	= (1 << 0),	/* is slot in swap_info[] used? */
-	SWP_WRITEOK	= (1 << 1),	/* ok to write to this swap?	*/
-	SWP_DISCARDABLE = (1 << 2),	/* blkdev support discard */
-	SWP_DISCARDING	= (1 << 3),	/* now discarding a free cluster */
-	SWP_SOLIDSTATE	= (1 << 4),	/* blkdev seeks are cheap */
-	SWP_CONTINUED	= (1 << 5),	/* swap_map has count continuation */
-	SWP_BLKDEV	= (1 << 6),	/* its a block device */
-	SWP_ACTIVATED	= (1 << 7),	/* set after swap_activate success */
-	SWP_FS_OPS	= (1 << 8),	/* swapfile operations go through fs */
-	SWP_AREA_DISCARD = (1 << 9),	/* single-time swap area discards */
-	SWP_PAGE_DISCARD = (1 << 10),	/* freed swap page-cluster discards */
-	SWP_STABLE_WRITES = (1 << 11),	/* no overwrite PG_writeback pages */
-	SWP_SYNCHRONOUS_IO = (1 << 12),	/* synchronous IO is efficient */
-					/* add others here before... */
-	SWP_SCANNING	= (1 << 14),	/* refcount in scan_swap_map */
+	SWP_USED	= (1 << 0),		/* is slot in swap_info[] used? */
+	SWP_WRITEOK	= (1 << 1),		/* ok to write to this swap?	*/
+	SWP_DISCARDABLE = (1 << 2),		/* blkdev support discard */
+	SWP_DISCARDING	= (1 << 3),		/* now discarding a free cluster */
+	SWP_SOLIDSTATE	= (1 << 4),		/* blkdev seeks are cheap */
+	SWP_CONTINUED	= (1 << 5),		/* swap_map has count continuation */
+	SWP_BLKDEV	= (1 << 6),		/* its a block device */
+	SWP_ACTIVATED	= (1 << 7),		/* set after swap_activate success */
+	SWP_FS_OPS	= (1 << 8),		/* swapfile operations go through fs */
+	SWP_AREA_DISCARD = (1 << 9),		/* single-time swap area discards */
+	SWP_PAGE_DISCARD = (1 << 10),		/* freed swap page-cluster discards */
+	SWP_STABLE_WRITES = (1 << 11),		/* no overwrite PG_writeback pages */
+	SWP_SYNCHRONOUS_IO = (1 << 12),		/* synchronous IO is efficient */
+	SWP_READ_SYNCHRONOUS_IO = (1 << 13),	/* synchronous IO is efficient */
+						/* add others here before... */
+	SWP_SCANNING	= (1 << 14),		/* refcount in scan_swap_map */
 };
 
 #define SWAP_CLUSTER_MAX 32UL
diff --git a/mm/memory.c b/mm/memory.c
index ebfc9768f801..f531a6bfea5b 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4089,7 +4089,8 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
 	swapcache = folio;
 
 	if (!folio) {
-		if (data_race(si->flags & SWP_SYNCHRONOUS_IO) &&
+		if ((data_race(si->flags & (SWP_SYNCHRONOUS_IO |
+		    SWP_READ_SYNCHRONOUS_IO))) &&
 		    __swap_count(entry) == 1) {
 			/*
 			 * Prevent parallel swapin from proceeding with
diff --git a/mm/page_io.c b/mm/page_io.c
index ff8c99ee3af7..98a00709e98c 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c
@@ -499,7 +499,7 @@ static void swap_read_folio_bdev_async(struct folio *folio,
 void swap_read_folio(struct folio *folio, struct swap_iocb **plug)
 {
 	struct swap_info_struct *sis = swp_swap_info(folio->swap);
-	bool synchronous = sis->flags & SWP_SYNCHRONOUS_IO;
+	bool synchronous = sis->flags & (SWP_SYNCHRONOUS_IO | SWP_READ_SYNCHRONOUS_IO);
 	bool workingset = folio_test_workingset(folio);
 	unsigned long pflags;
 	bool in_thrashing;
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 38bdc439651a..7b8feb235aab 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -3177,6 +3177,9 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
 	if (p->bdev && bdev_synchronous(p->bdev))
 		p->flags |= SWP_SYNCHRONOUS_IO;
 
+	if (p->bdev && bdev_read_synchronous(p->bdev))
+		p->flags |= SWP_READ_SYNCHRONOUS_IO;
+
 	if (p->bdev && bdev_nonrot(p->bdev)) {
 		int cpu, i;
 		unsigned long ci, nr_cluster;
-- 
2.45.2

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

[Christoph Hellwig]

Well, you're not actually setting your new flags anywhere, which -
as you might know - is an reson for an insta-NAK.

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

[Chris Li]

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

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

[Qun-wei Lin (林群崴)]

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

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

[Huang, Ying]

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

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

[Qun-wei Lin (林群崴)]

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

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

[Matthew Wilcox]

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?