Hello, Adam. On Fri, Sep 30, 2016 at 09:02:17AM -0700, Adam Manzanares wrote: > I'll start with the changes I made and work my way through a grep of > > ioprio. Please add or correct any of the assumptions I have made. >
Well, it looks like you're the one who's most familiar with ioprio handling at this point. :) > In blk-core, the behavior before the patch is to get the ioprio for the > request > from the bio. The only references I found to bio_set_prio are in bcache. Both > > of these references are in low priority operations (gc, bg writeback) so the > > iopriority of the bio is set to IO_PRIOCLASS_IDLE in these cases. > > > > A kernel thread is used to submit these bios so the ioprio is going to come > > from the current running task if the iocontext exists. This could be a > problem > if we have set a task with high priority and some background work ends up > > getting generated in the bcache layer. I propose that we check if the > > iopriority of the bio is valid and if so, then we keep the priorirty from the > > bio. > I wonder whether the right thing to do is adding bio->bi_ioprio which is initialized on bio submission and carried through req->ioprio. > The second area that I see a potential problem is in the merging code code in > > blk-core when a bio is queued. If there is a request that is mergeable then > > the merge code takes the highest priority of the bio and the request. This > > could wipe out the values set by bio_set_prio. I think it would be > > best to set the request as non mergeable when we see that it is a high > > priority IO request. > The current behavior should be fine for most non-pathological cases but I have no objection to not merging ios with differing priorities. > The third area that is of interest is in the CFQ scheduler and the ioprio is > > only used in the case of async IO and I found that the priority is only > > obtained from the task and not from the request. This leads me to believe > that > the changes made in the blk-core to add the priority to the request will not > > impact the CFQ scheduler. > > The fourth area that might be concerning is the drivers. virtio_block copies > > the request priority into a virtual block request. I am assuming that this > > eventually makes it to another device driver so we don't need to worry about > > this. null block device driver also uses the ioprio, but this is also not a > > concern. lightnvm also sets the ioprio to build a request that is passed onto > > another driver. The last driver that uses the request ioprio is the fusion > > mptsas driver and I don't understand how it is using the ioprio. From what I > > can tell it is taking a request of IOPRIO_CLASS_NONE with data of 0x7 and > > calling this high priority IO. This could be impacted by the code I have > > proposed, but I believe the authors intended to treat this particular ioprio > > value as high priority. The driver will pass the request to the device > > with high priority if the appropriate ioprio values is seen on the request. > > > > The fifth area that I noticed may be impacted is file systems. btrfs uses low > > priority IO for read ahead. Ext4 uses ioprio for journaling. Both of these > > issues are not a problem because the ioprio is set on the task and not on a > > bio. Yeah, looks good to me. Care to include a brief summary of expected (non)impacts in the patch description? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
