[dm-devel] [dm-crypt] Hang problem with dm-crypt

[Milan Broz]

On 09/26/2016 03:08 PM, Yu, Wenqian wrote:
> Hi, Milan,
> 
> Thanks for the detail information. I noticed the comments and the underlying design logic.
> 
> In dm-crypt existing design, there is an assumption that the acceleration driver can queue the requests which are not sent to hardware.  
> 
> I think there are at least two scenarios we should consider to make it more robust.
> 1.  The queue is full even if the driver has the ability to queue a number of the requests.
> 2.  The acceleration hardware/driver doesn't have the ability to queue the requests.
> 
> Should we add other error code to handle this?

I would prefer to ask on crypto API mailing list how the interface is expected to behave
(if the queue/REQ_MAY_BACKLOG is mandatory etc) before complicating any logic in dmcrypt.

Also please send any possible patches to dm-devel mailing list (I added cc there now).

Thanks,
Milan

>  
> Thanks,
> - Wenqian
> 
> -----Original Message-----
> From: Milan Broz [mailto:gmazyland at gmail.com] 
> Sent: Monday, September 26, 2016 6:28 PM
> To: Yu, Wenqian; dm-crypt at saout.de
> Subject: Re: [dm-crypt] Hang problem with dm-crypt
> 
> On 09/26/2016 08:50 AM, Yu, Wenqian wrote:
>> I tried to use dm-crypt for disk encryption with accelerators and 
>> found that it will hang when accelerator returned EBUSY, which means 
>> the driver request queue is full.
> 
> That is normal state, when request is processed asynchronously later.
> 
> Please read explicit comments in code we added to understand this logic.
> added in this commit:
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/md/dm-crypt.c?id=54cea3f6681ad9360814e2926d1f723bbd0f74ed
>  
>> Per the logic in crypt_convert(), the request will be skipped if the 
>> request is not sent to crypto driver when the driver request queue is 
>> full.  Is this expected behavior?
> 
> It is not skipped, it is queued (or it waits if queue is full and then processes as asynchronous branch (EINPROGRESS))
> 
>> In crypt_convert_block(), the sector is advanced (bio_advance_iter()) 
>> no matter whether crypto_skcipher_encrypt()/crypto_skcipher_decrypt()
>> send the request to accelerator driver or not.   When the driver
>> request queue is full, EBUSY will be returned from 
>> crypto_skcipher_encrypt()/crypto_skcipher_decrypt().  And in 
>> crypt_convert(), the existing implementation is waiting for a 
>> completion from a request, which is not queued in the driver when 
>> EBUSY is encountered from crypt_convert_block ().  In this case, the 
>> sector should not be advanced or should be rolled back as the request 
>> is not sent to accelerator driver.
> 
> I think it should be queued (IOW the one that returns BUSY should be queued).
> If it is not done, I would say it is bug in acceleration driver.
> Note this flag:
> /*
>  * Use REQ_MAY_BACKLOG so a cipher driver internally backlogs
>  * requests if driver request queue is full.
>  */
> 
> Anyway, this is more question for crypto API mailing list...
> I think that dmcrypt processing is correct here.
> 
> Milan
> _______________________________________________
> dm-crypt mailing list
> dm-crypt at saout.de
> http://www.saout.de/mailman/listinfo/dm-crypt
>