| Message ID | 20230726193440.1655149-2-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve the performance of F2FS on zoned UFS | expand |
On 7/27/23 04:34, Bart Van Assche wrote: > Writes in sequential write required zones must happen at the write > pointer. Even if the submitter of the write commands (e.g. a filesystem) > submits writes for sequential write required zones in order, the block > layer or the storage controller may reorder these write commands. > > The zone locking mechanism in the mq-deadline I/O scheduler serializes > write commands for sequential zones. Some but not all storage controllers > require this serialization. Introduce a new flag such that block drivers > can request that zone write locking is disabled. For the last sentence: Introduce the new flag QUEUE_FLAG_NO_ZONE_WRITE_LOCK to allow block device drivers to indicate that they can preserve wrtie command ordering and thus do not require zone write locking to be used. Is in my opinion a lot clearer. > > Cc: Christoph Hellwig <hch@lst.de> > Cc: Damien Le Moal <dlemoal@kernel.org> > Cc: Ming Lei <ming.lei@redhat.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > include/linux/blkdev.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h > index 2f5371b8482c..066ac395f62f 100644 > --- a/include/linux/blkdev.h > +++ b/include/linux/blkdev.h > @@ -534,6 +534,11 @@ struct request_queue { > #define QUEUE_FLAG_NONROT 6 /* non-rotational device (SSD) */ > #define QUEUE_FLAG_VIRT QUEUE_FLAG_NONROT /* paravirt device */ > #define QUEUE_FLAG_IO_STAT 7 /* do disk/partitions IO accounting */ > +/* > + * Do not use the zone write lock for sequential writes for sequential write > + * required zones. > + */ > +#define QUEUE_FLAG_NO_ZONE_WRITE_LOCK 8 > #define QUEUE_FLAG_NOXMERGES 9 /* No extended merges */ > #define QUEUE_FLAG_ADD_RANDOM 10 /* Contributes to random pool */ > #define QUEUE_FLAG_SYNCHRONOUS 11 /* always completes in submit context */ > @@ -597,6 +602,11 @@ bool blk_queue_flag_test_and_set(unsigned int flag, struct request_queue *q); > #define blk_queue_skip_tagset_quiesce(q) \ > test_bit(QUEUE_FLAG_SKIP_TAGSET_QUIESCE, &(q)->queue_flags) > > +static inline bool blk_queue_no_zone_write_lock(struct request_queue *q) > +{ > + return test_bit(QUEUE_FLAG_NO_ZONE_WRITE_LOCK, &q->queue_flags); > +} > + > extern void blk_set_pm_only(struct request_queue *q); > extern void blk_clear_pm_only(struct request_queue *q); Looks good, but I really think this should be squashed with patch 2. More on this in reply to that patch. >
On 7/26/2023 12:34 PM, Bart Van Assche wrote: > Writes in sequential write required zones must happen at the write > pointer. Even if the submitter of the write commands (e.g. a filesystem) > submits writes for sequential write required zones in order, the block > layer or the storage controller may reorder these write commands. > > The zone locking mechanism in the mq-deadline I/O scheduler serializes > write commands for sequential zones. Some but not all storage controllers > require this serialization. Introduce a new flag such that block drivers > can request that zone write locking is disabled. > > Cc: Christoph Hellwig <hch@lst.de> > Cc: Damien Le Moal <dlemoal@kernel.org> > Cc: Ming Lei <ming.lei@redhat.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > include/linux/blkdev.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h > index 2f5371b8482c..066ac395f62f 100644 > --- a/include/linux/blkdev.h > +++ b/include/linux/blkdev.h > @@ -534,6 +534,11 @@ struct request_queue { > #define QUEUE_FLAG_NONROT 6 /* non-rotational device (SSD) */ > #define QUEUE_FLAG_VIRT QUEUE_FLAG_NONROT /* paravirt device */ > #define QUEUE_FLAG_IO_STAT 7 /* do disk/partitions IO accounting */ > +/* > + * Do not use the zone write lock for sequential writes for sequential write > + * required zones. > + */ In first go I got little confused with above comment, how about :- /* * When issuing sequential writes on zone type * BLK_ZONE_TYPE_SEQWRITE_REQ, don't use zone write locking. */ It makes it easier to search in code with BLK_ZONE_TYPE_SEQWRITE_REQ but if everyone else is okay with original comment or if it is not correct suggestion feel free to ignore my suggestion ... > +#define QUEUE_FLAG_NO_ZONE_WRITE_LOCK 8 > #define QUEUE_FLAG_NOXMERGES 9 /* No extended merges */ > #define QUEUE_FLAG_ADD_RANDOM 10 /* Contributes to random pool */ > #define QUEUE_FLAG_SYNCHRONOUS 11 /* always completes in submit context */ > @@ -597,6 +602,11 @@ bool blk_queue_flag_test_and_set(unsigned int flag, struct request_queue *q); > #define blk_queue_skip_tagset_quiesce(q) \ > test_bit(QUEUE_FLAG_SKIP_TAGSET_QUIESCE, &(q)->queue_flags) > > +static inline bool blk_queue_no_zone_write_lock(struct request_queue *q) > +{ > + return test_bit(QUEUE_FLAG_NO_ZONE_WRITE_LOCK, &q->queue_flags); > +} > + Is it true above helper is only called from blk_no_zone_write_lock() ? if that is true open coding above call into blk_no_zone_write_lock() makes it more readable to me while reviewing the series, but other are okay then ignore my comment.. > extern void blk_set_pm_only(struct request_queue *q); > extern void blk_clear_pm_only(struct request_queue *q); > -ck
On Thu, Jul 27, 2023 at 1:39 AM Bart Van Assche <bvanassche@acm.org> wrote: > + * Do not use the zone write lock for sequential writes for sequential write > + * required zones. > + */ > +#define QUEUE_FLAG_NO_ZONE_WRITE_LOCK 8 Instead of QUEUE_FLAG_NO_ZONE_WRITE_LOCK I feel QUEUE_FLAG_SKIP_ZONE_WRITE_LOCK is better. --Nitesh Shetty
On 7/27/23 03:59, Nitesh Shetty wrote: > On Thu, Jul 27, 2023 at 1:39 AM Bart Van Assche <bvanassche@acm.org> wrote: >> + * Do not use the zone write lock for sequential writes for sequential write >> + * required zones. >> + */ >> +#define QUEUE_FLAG_NO_ZONE_WRITE_LOCK 8 > > Instead of QUEUE_FLAG_NO_ZONE_WRITE_LOCK I feel > QUEUE_FLAG_SKIP_ZONE_WRITE_LOCK is better. Hmm ... this patch series makes it possible to use an UFS storage controller and a zoned UFS device with no I/O scheduler attached. If no I/O scheduler is active, there is no zone write locking to skip so I think that the former name is better. Thanks, Bart.
On Thu, Jul 27, 2023 at 8:13 PM Bart Van Assche <bvanassche@acm.org> wrote: > > On 7/27/23 03:59, Nitesh Shetty wrote: > > On Thu, Jul 27, 2023 at 1:39 AM Bart Van Assche <bvanassche@acm.org> wrote: > >> + * Do not use the zone write lock for sequential writes for sequential write > >> + * required zones. > >> + */ > >> +#define QUEUE_FLAG_NO_ZONE_WRITE_LOCK 8 > > > > Instead of QUEUE_FLAG_NO_ZONE_WRITE_LOCK I feel > > QUEUE_FLAG_SKIP_ZONE_WRITE_LOCK is better. > > Hmm ... this patch series makes it possible to use an UFS storage > controller and a zoned UFS device with no I/O scheduler attached. If no > I/O scheduler is active, there is no zone write locking to skip so I > think that the former name is better. Okay, I missed no I/O scheduler. Thanks, Nitesh Shetty
On 7/26/23 17:54, Chaitanya Kulkarni wrote: > On 7/26/2023 12:34 PM, Bart Van Assche wrote: >> +/* >> + * Do not use the zone write lock for sequential writes for sequential write >> + * required zones. >> + */ > > In first go I got little confused with above comment, how about :- > > /* > * When issuing sequential writes on zone type > * BLK_ZONE_TYPE_SEQWRITE_REQ, don't use zone write locking. > */ > > It makes it easier to search in code with BLK_ZONE_TYPE_SEQWRITE_REQ > but if everyone else is okay with original comment or if it is not > correct suggestion feel free to ignore my suggestion ... How about this comment? /* * Do not serialize sequential writes sent to a sequential write * required zone (BLK_ZONE_TYPE_SEQWRITE_REQ). */ Thanks, Bart.
On 7/27/2023 1:20 PM, Bart Van Assche wrote: > On 7/26/23 17:54, Chaitanya Kulkarni wrote: >> On 7/26/2023 12:34 PM, Bart Van Assche wrote: >>> +/* >>> + * Do not use the zone write lock for sequential writes for >>> sequential write >>> + * required zones. >>> + */ >> >> In first go I got little confused with above comment, how about :- >> >> /* >> * When issuing sequential writes on zone type >> * BLK_ZONE_TYPE_SEQWRITE_REQ, don't use zone write locking. >> */ >> >> It makes it easier to search in code with BLK_ZONE_TYPE_SEQWRITE_REQ >> but if everyone else is okay with original comment or if it is not >> correct suggestion feel free to ignore my suggestion ... > > How about this comment? > > /* > * Do not serialize sequential writes sent to a sequential write > * required zone (BLK_ZONE_TYPE_SEQWRITE_REQ). > */ > > Thanks, > > Bart. > works for me .. -ck
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 2f5371b8482c..066ac395f62f 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -534,6 +534,11 @@ struct request_queue { #define QUEUE_FLAG_NONROT 6 /* non-rotational device (SSD) */ #define QUEUE_FLAG_VIRT QUEUE_FLAG_NONROT /* paravirt device */ #define QUEUE_FLAG_IO_STAT 7 /* do disk/partitions IO accounting */ +/* + * Do not use the zone write lock for sequential writes for sequential write + * required zones. + */ +#define QUEUE_FLAG_NO_ZONE_WRITE_LOCK 8 #define QUEUE_FLAG_NOXMERGES 9 /* No extended merges */ #define QUEUE_FLAG_ADD_RANDOM 10 /* Contributes to random pool */ #define QUEUE_FLAG_SYNCHRONOUS 11 /* always completes in submit context */ @@ -597,6 +602,11 @@ bool blk_queue_flag_test_and_set(unsigned int flag, struct request_queue *q); #define blk_queue_skip_tagset_quiesce(q) \ test_bit(QUEUE_FLAG_SKIP_TAGSET_QUIESCE, &(q)->queue_flags) +static inline bool blk_queue_no_zone_write_lock(struct request_queue *q) +{ + return test_bit(QUEUE_FLAG_NO_ZONE_WRITE_LOCK, &q->queue_flags); +} + extern void blk_set_pm_only(struct request_queue *q); extern void blk_clear_pm_only(struct request_queue *q);
Writes in sequential write required zones must happen at the write pointer. Even if the submitter of the write commands (e.g. a filesystem) submits writes for sequential write required zones in order, the block layer or the storage controller may reorder these write commands. The zone locking mechanism in the mq-deadline I/O scheduler serializes write commands for sequential zones. Some but not all storage controllers require this serialization. Introduce a new flag such that block drivers can request that zone write locking is disabled. Cc: Christoph Hellwig <hch@lst.de> Cc: Damien Le Moal <dlemoal@kernel.org> Cc: Ming Lei <ming.lei@redhat.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- include/linux/blkdev.h | 10 ++++++++++ 1 file changed, 10 insertions(+)