Re: [External] Re: [PATCH v3] blk-throtl: Introduce sync and async queues for blk-throtl

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





在 2023/1/5 上午6:11, Tejun Heo 写道:
Hello,

On Mon, Dec 26, 2022 at 09:05:05PM +0800, Jinke Han wrote:
  static void throtl_pending_timer_fn(struct timer_list *t);
+static inline struct bio *throtl_qnode_bio_list_pop(struct throtl_qnode *qn);

Just define it before the first usage? Also, I think it'd be fine to let the
compiler decide whether to inline.

+#define BLK_THROTL_SYNC(bio) (bio->bi_opf & (REQ_SYNC | REQ_META | REQ_PRIO))

Nitpick but the above is used only in one place. Does it help to define it
as a macro?

+/**
+ * throtl_qnode_bio_peek - peek a bio for a qn
+ * @qn: the qnode to peek from
+ *
+ * For read qn, just peek bio from the SYNC queue and return.
+ * For write qn, we first ask the next_to_disp for bio and will pop a bio
+ * to fill it if it's NULL. The next_to_disp is used to pin the bio for
+ * next to dispatch. It is necessary. In the dispatching  process, a peeked
+ * bio may can't be dispatched due to lack of budget and has to wait, the
+ * dispatching process may give up and the spin lock of the request queue
+ * will be released. New bio may be queued in as the spin lock were released.
+ * When it's time to dispatch the waiting bio, another bio may be selected to
+ * check the limit and may be dispatched. If the dispatched bio is smaller
+ * than the waiting bio, the bandwidth may be hard to satisfied as we may
+ * trim the slice after each dispatch.
+ * So pinning the next_to_disp to make sure that the waiting bio and the
+ * dispatched one later always the same one in case that the spin lock of
+ * queue was released and re-holded.

Can you please format it better and proof-read it. I can mostly understand
what it's saying but it can be improved quite a bit. Can you elaborate the
starvation scenario further? What about the [a]sync queue split makes this
more likely?

+/**
+ * throtl_qnode_bio_pop: pop a bio from sync/async queue
+ * @qn: the qnode to pop a bio from
+ *
+ * For write io qn, the target queue to pop was determined by the disp_sync_cnt.
+ * Try to pop bio from target queue, fetch the bio and return it when it is not
+ * empty. If the target queue empty, pop bio from another queue instead.

How about:

         For reads, always pop from the ASYNC queue. For writes, target SYNC
         or ASYNC queue based on disp_sync_cnt. If empty, try the other
         queue.

+static inline struct bio *throtl_qnode_bio_list_pop(struct throtl_qnode *qn)
+{
+	struct bio *bio;
+	int from = SYNC;
+
+	if (qn->disp_sync_cnt == THROTL_SYNC_FACTOR)
+		from = ASYNC;

?: often is less readable but I wonder whether it'd be more readable here:

         from = qn->disp_sync_cnt == THROTL_SYNC_FACTOR ? ASYNC : SYNC;

+
+	bio = bio_list_pop(&qn->bios[from]);
+	if (!bio) {
+		from = 1 - from;
+		bio = bio_list_pop(&qn->bios[from]);
+	}
+
+	if ((qn->disp_sync_cnt < THROTL_SYNC_FACTOR) &&
+		(from == SYNC))

Why the line break? Also, this may be more personal preference but I'm not
sure the parentheses are helping much here.

+		qn->disp_sync_cnt++;
+	else
+		qn->disp_sync_cnt = 0;
+
+	return bio;
+}

Thanks.


Thanks. Your suggestion is detailed and helpful. I will accept it and send the v4.

Thanks.



[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux