On Sat, 27 May 2017, Coly Li wrote: > On 2017/5/26 下午5:17, tang.junhui@xxxxxxxxxx wrote: > > Hello Coly, > > > > > > ＞ bcache_device_init() already does something overlapped with > > > > ＞ blk_queue_stack_limits(), which means your patch introduces redundant > > > > ＞ code execution. Also blk_queue_stack_limits() may fail, so if you do > > > > ＞ want to add this function call, you also need to check the return value > > > > ＞ and handle the failure condition. > > > > Queue Limits are assigned to default value in bcache_device_init(), and then > > > > updated by inheriting underlying driver in blk_queue_stack_limits(), so > > I think > > > > there is no redundant or contradiction between them. > > > > blk_queue_stack_limits() is defined as a void function, so we have no > > need to > > > > check the return value. > > > > blk_queue_stack_limits() is wrapper of blk_stack_limits(). If you look > into blk_stack_limits() you will find the redundant code I meant, as > well as the return value I suggested to check. > > > > > > ＞ Currently I don't see benefit from this patch. It is probably I miss > > > > something from your patch, please correct me if you think so. > > > > Please consider this scenario: Bcache driver show a large max_sectors_kb, > > > > then upper layer would merger seqention IOs to make the size of BIO as > > bigger > > > > as near to max_sectors_kb. But actually, bcache driver can not send this > > BIO to > > > > underlying driver, and have to split the big IO to smaller ones. > > > > I think these are useless work. > > Oh, maybe I know what you mean. > It is very rare for the above scenario. Maybe I am wrong, but could you > please provide a real example for the above situation ? What stacking considerations should be made about the caching device vs. backing device here? Would you choose the minimum of both device stacking values? Does blk_(queue_)stack_limits already handle this by checking the blockdev references held by the bcache device? -- Eric Wheeler > > Thanks. > > -- > Coly Li >
