Correctly locking a Block Device Request Handler

Correctly locking a Block Device Request Handler

[Marcel Müller]

Hello everyone,

I'm currently writing a block device driver and got stuck at trying to
understand how to correctly handle the locking
in the reqfn one passes to `blk_init_queue`.

My code looks like this:

    static DEFINE_SPINLOCK(rblk_lock);

    /* Code */

    static void rblk_request_handler(struct request_queue *q)
    __releases(q->queue_lock) __acquires(q->queue_lock)
    {
        struct request* req;
        unsigned long flags = 0;
   
        printk(KERN_INFO "rblk: Got request(s) \n");
        while ((req = blk_fetch_request(q)) != NULL) {
            printk(KERN_INFO "rblk: Handling request \n"); // <- Gets
printed
            spin_lock_irqsave(q->queue_lock, flags);
            blk_end_request_all(req, -ENOTTY);
            spin_unlock_irqrestore(q->queue_lock, flags);
            printk(KERN_INFO "rblk: Handled request \n"); // <- Does not
get printed
        }
    }


    static in rblk_init() {
        /* Get major number, allocate devices */
        for (i = 0; i < rblk_cnt; i++) { // For each device
            /* alloc_disk, check for allocation fail */

            disk->queue = blk_init_queue(rblk_request_handler, &rblk_lock);
        }
    }


This didn't work, and it was obvious to me that it was hanging in the
spinlock. So I tried
removing the locking, (this SO answer says that the queue is already
locked:
https://stackoverflow.com/questions/19418979/proper-way-to-lock-the-queue-in-a-block-device-driver-while-serving-requests)
however, I still get the problem that the queue locks up and the second
message never hits the message
queue.

What /does/ work is if I invert the order of locking. As in, unlock
first, end_request and then lock again.
However that doesn't seem to be the correct way. What am I doing
completely wrong, what did I misunderstand?

Full code: https://gist.github.com/TheNeikos/8798788defa1a9f316e6

Thanks

Correctly locking a Block Device Request Handler

[Pranay Srivastava]

Hi

On Mon, Nov 16, 2015 at 10:02 PM, Marcel M?ller <neikos@neikos.email> wrote:
> Hello everyone,
>
> I'm currently writing a block device driver and got stuck at trying to
> understand how to correctly handle the locking
> in the reqfn one passes to `blk_init_queue`.
>
> My code looks like this:
>
>     static DEFINE_SPINLOCK(rblk_lock);
>
>     /* Code */
>
>     static void rblk_request_handler(struct request_queue *q)
>     __releases(q->queue_lock) __acquires(q->queue_lock)

as per code, the queue_lock would be held before it enters your
request_func. So no you don't need
to lock it here.

If you must however need to do some stuff that requires the
spin_lock_* to be released, you must
make sure that before you leave this function you have reacquired that lock.

>     {
>         struct request* req;
>         unsigned long flags = 0;
>
>         printk(KERN_INFO "rblk: Got request(s) \n");
>         while ((req = blk_fetch_request(q)) != NULL) {
>             printk(KERN_INFO "rblk: Handling request \n"); // <- Gets
> printed

This isn't required if this is your request_function. Let's say you
just want to consume the requests
but want to actually handle them elsewhere then you may require to
take the spin_lock_* over there.


>             spin_lock_irqsave(q->queue_lock, flags);

You are generating error for all requests?

>             blk_end_request_all(req, -ENOTTY);
>             spin_unlock_irqrestore(q->queue_lock, flags);
>             printk(KERN_INFO "rblk: Handled request \n"); // <- Does not
> get printed
>         }
>     }
>
>
>     static in rblk_init() {
>         /* Get major number, allocate devices */
>         for (i = 0; i < rblk_cnt; i++) { // For each device
>             /* alloc_disk, check for allocation fail */
>
>             disk->queue = blk_init_queue(rblk_request_handler, &rblk_lock);
>         }
>     }
>
>
> This didn't work, and it was obvious to me that it was hanging in the
> spinlock. So I tried
> removing the locking, (this SO answer says that the queue is already
> locked:
> https://stackoverflow.com/questions/19418979/proper-way-to-lock-the-queue-in-a-block-device-driver-while-serving-requests)
> however, I still get the problem that the queue locks up and the second
> message never hits the message
> queue.
>
> What /does/ work is if I invert the order of locking. As in, unlock
> first, end_request and then lock again.
> However that doesn't seem to be the correct way. What am I doing
> completely wrong, what did I misunderstand?
>
> Full code: https://gist.github.com/TheNeikos/8798788defa1a9f316e6

You can check one here https://github.com/pranjas/block_driver. No
real device though.
>
> Thanks
>
>
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies



-- 
        ---P.K.S

Correctly locking a Block Device Request Handler

[Marcel Müller]

On 11/18/2015 07:11 AM, Pranay Srivastava wrote:
> Hi
>
> On Mon, Nov 16, 2015 at 10:02 PM, Marcel M?ller <neikos@neikos.email> wrote:
>> Hello everyone,
>>
>> I'm currently writing a block device driver and got stuck at trying to
>> understand how to correctly handle the locking
>> in the reqfn one passes to `blk_init_queue`.
>>
>> My code looks like this:
>>
>>     static DEFINE_SPINLOCK(rblk_lock);
>>
>>     /* Code */
>>
>>     static void rblk_request_handler(struct request_queue *q)
>>     __releases(q->queue_lock) __acquires(q->queue_lock)
> as per code, the queue_lock would be held before it enters your
> request_func. So no you don't need
> to lock it here.
>
> If you must however need to do some stuff that requires the
> spin_lock_* to be released, you must
> make sure that before you leave this function you have reacquired that lock.
>
>>     {
>>         struct request* req;
>>         unsigned long flags = 0;
>>
>>         printk(KERN_INFO "rblk: Got request(s) \n");
>>         while ((req = blk_fetch_request(q)) != NULL) {
>>             printk(KERN_INFO "rblk: Handling request \n"); // <- Gets
>> printed
> This isn't required if this is your request_function. Let's say you
> just want to consume the requests
> but want to actually handle them elsewhere then you may require to
> take the spin_lock_* over there.
>
>
>>             spin_lock_irqsave(q->queue_lock, flags);
> You are generating error for all requests?
>
>>             blk_end_request_all(req, -ENOTTY);
>>             spin_unlock_irqrestore(q->queue_lock, flags);
>>             printk(KERN_INFO "rblk: Handled request \n"); // <- Does not
>> get printed
>>         }
>>     }
>>
>>
>>     static in rblk_init() {
>>         /* Get major number, allocate devices */
>>         for (i = 0; i < rblk_cnt; i++) { // For each device
>>             /* alloc_disk, check for allocation fail */
>>
>>             disk->queue = blk_init_queue(rblk_request_handler, &rblk_lock);
>>         }
>>     }
>>
>>
>> This didn't work, and it was obvious to me that it was hanging in the
>> spinlock. So I tried
>> removing the locking, (this SO answer says that the queue is already
>> locked:
>> https://stackoverflow.com/questions/19418979/proper-way-to-lock-the-queue-in-a-block-device-driver-while-serving-requests)
>> however, I still get the problem that the queue locks up and the second
>> message never hits the message
>> queue.
>>
>> What /does/ work is if I invert the order of locking. As in, unlock
>> first, end_request and then lock again.
>> However that doesn't seem to be the correct way. What am I doing
>> completely wrong, what did I misunderstand?
>>
>> Full code: https://gist.github.com/TheNeikos/8798788defa1a9f316e6
> You can check one here https://github.com/pranjas/block_driver. No
> real device though.
>> Thanks
>>
>>
>> _______________________________________________
>> Kernelnewbies mailing list
>> Kernelnewbies at kernelnewbies.org
>> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
>
>
Hello,

Thank you! Your explanations and code showed me some new things one can
do, so
that definitely helped!

- Marcel