On Wed, Jun 28, 2023 at 12:55:49PM +0800, Chengming Zhou wrote: > On 2023/6/28 12:13, Ming Lei wrote: > > On Tue, Jun 27, 2023 at 08:08:52PM +0800, chengming.zhou@xxxxxxxxx wrote: > >> From: Chengming Zhou <zhouchengming@xxxxxxxxxxxxx> > >> > >> The flush state machine use a double list to link all inflight > >> flush_data requests, to avoid issuing separate post-flushes for > >> these flush_data requests which shared PREFLUSH. > >> > >> So we can't reuse rq->queuelist, this is why we need rq->flush.list > >> > >> In preparation of the next patch that reuse rq->queuelist for flush > >> state machine, we change the double linked list to a u64 counter, > >> which count all inflight flush_data requests. > >> > >> This is ok since we only need to know if there is any inflight > >> flush_data request, so a u64 counter is good. The only problem I can > >> think of is that u64 counter may overflow, which should be unlikely happen. > > > > It won't overflow, q->nr_requests is 'unsigned long', which should have > > been limited to one more reasonable value, such as 2 * BLK_MQ_MAX_DEPTH, so > > u16 should be big enough in theory. > > Ah, right. q->nr_requests is 'unsigned long' and q->queue_depth is 'unsigned int', > so 'unsigned long' counter here won't overflow. Not like q->nr_requests, q->queue_depth usually means the whole queue's depth, which may cover all hw queue's depth. And it is only used by scsi, but it should be held in "unsigned short" too. > > Should I change it to smaller 'unsigned short' or just leave it as 'unsigned long' ? > (Now the size of struct blk_flush_queue is exactly 64 bytes) You have to limit q->nr_requests first, which may need a bit more work for avoiding compiling warning or sort of thing. And 64k is big enough for holding per-queue scheduler request. Once it is done, it is fine to define this counter as 'unsigned short'. Thanks, Ming
