On 02/03/2017 11:43 AM, Christoph Hellwig wrote:
>> typedef struct sg_request { /* SG_MAX_QUEUE requests outstanding per file */
>> - struct sg_request *nextrp; /* NULL -> tail request (slist) */
>> + struct list_head nextrp; /* list entry */
>
> s/nextrp/entry/
>
>> @@ -2078,16 +2076,13 @@ static long sg_compat_ioctl(struct file *filp,
>> unsigned int cmd_in, unsigned lon
>> if (k < SG_MAX_QUEUE) {
>> memset(rp, 0, sizeof (Sg_request));
>> rp->parentfp = sfp;
>> + list_add(&rp->nextrp, &sfp->rq_list);
>
> The old code did a tail insertation. And this whole function should
> become a lot simpler with proper lists anyway:
>
Yeah, thought about that, too, but then I just went for the sloppy
approach to minimize changes.
> static Sg_request *
> sg_add_request(Sg_fd * sfp)
> {
> int k;
> unsigned long iflags;
> Sg_request *rp = sfp->req_arr;
>
> write_lock_irqsave(&sfp->rq_list_lock, iflags);
> if (!list_empty(&sfp->rq_list)) {
> if (!sfp->cmd_q)
> goto out_unlock;
>
> for (k = 0; k < SG_MAX_QUEUE; ++k, ++rp) {
> if (!rp->parentfp)
> break;
> }
> if (k >= SG_MAX_QUEUE)
> goto out_unlock;
> }
>
> memset(rp, 0, sizeof (Sg_request));
> rp->parentfp = sfp;
> rp->header.duration = jiffies_to_msecs(jiffies);
> list_add_tail(&rp->nextrp, &sfp->rq_list);
> write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
> return rp;
>
> out_unlock:
> write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
> return NULL;
>
Okay, will be updating the patch.
>> + if ((!sfp) || (!srp) || (list_empty(&sfp->rq_list)))
>
> No need for all these braces.
>
Okay.
>> + if (!list_empty(&srp->nextrp)) {
>> + list_del_init(&srp->nextrp);
>
> I don't think we need the _init as we never check for an empty entry.
>
Yes.
>> {
>> struct sg_fd *sfp = container_of(work, struct sg_fd, ew.work);
>> struct sg_device *sdp = sfp->parentdp;
>> + Sg_request *srp, *tmp;
>>
>> /* Cleanup any responses which were never read(). */
>> - while (sfp->headrp)
>> - sg_finish_rem_req(sfp->headrp);
>> + list_for_each_entry_safe(srp, tmp, &sfp->rq_list, nextrp)
>> + sg_finish_rem_req(srp);
>
> What protects us from concurrent removals here?
>
Nothing.
But this patch is intended to just replace the hand-rolled list
implementation, not fixing bugs here.
The problem is that 'sg_finish_rem_req()' is taking the rq_list_lock,
so it needs a bit of rework to make that work properly.
But I'll give it a go.
> Either way I'd rather keep the whіle not empty style even with
> proper lists.
>
Okay.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
