From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa5.hgst.iphmx.com (esa5.hgst.iphmx.com [216.71.153.144]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.server123.net (Postfix) with ESMTPS for ; Fri, 25 Sep 2020 03:13:07 +0200 (CEST) From: Damien Le Moal Date: Fri, 25 Sep 2020 01:09:52 +0000 Message-ID: References: <1600281606-1446-1-git-send-email-sudhakar.panneerselvam@oracle.com> <3be1ea32-b6a8-41ef-a9ba-ed691434d068@default> <20200924012732.GA10766@redhat.com> <20200924051419.GA16103@sol.localdomain> <252587bb-c0b7-47c9-a97b-91422f8f9c47@default> <7b6fdfd5-0160-4bcf-b7ed-d0e51553c678@default> Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dm-crypt] [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sudhakar Panneerselvam , Mikulas Patocka Cc: "ssudhakarp@gmail.com" , Mike Snitzer , "dm-crypt@saout.de" , Eric Biggers , "dm-devel@redhat.com" , Shirley Ma , Martin Petersen , Milan Broz , "agk@redhat.com" On 2020/09/25 4:14, Sudhakar Panneerselvam wrote:=0A= >>=0A= >> On Thu, 24 Sep 2020, Sudhakar Panneerselvam wrote:=0A= >>=0A= >>>> By copying it to a temporary aligned buffer and issuing I/O on this=0A= >>>> buffer.=0A= >>>=0A= >>> I don't like this idea. Because, you need to allocate additional pages= =0A= >>> for the entire I/O size(for the misaligned case, if you think through= =0A= >>=0A= >> You can break the I/O to smaller pieces. You can use mempool for=0A= >> pre-allocation of the pages.=0A= > =0A= > Assuming we do this, how is this code simpler(based on your=0A= > comment below) than the fix in dm-crypt? In fact, this approach =0A= > would make the code change look bad in vhost, at the same time=0A= > having performance penalty. By doing this, we are just moving the =0A= > responsibility to other unrelated component.=0A= =0A= Because vhost is at the top of the block-io food chain. Fixing the unaligne= d=0A= segments there will ensure that it does not matter what device is under it.= It=0A= will work.=0A= =0A= I am still baffled that the unaligned segments go through in the first plac= e...=0A= Do we have something missing in the BIO code ?=0A= =0A= > =0A= >>=0A= >>> carefully, you will know why we have to allocate a temporary buffer tha= t=0A= >>> is as big as the original IO) and on top of it, copying the buffer from= =0A= >>> original to temporary buffer which is all unnecessary while it can=0A= >>> simply be fixed in dm-crypt without any of these additional overheads.= =0A= >>>=0A= >>>>=0A= >>>>> Only other=0A= >>>>> possibility I see is to have windows fix it by always sending 512 byt= e=0A= >>>>> aligned buffer lengths, but going with my earlier point that every ot= her=0A= >>>>> component in the Linux IO path handles this case well except for=0A= >>>>> dm-crypt, so it make more sense to fix it in dm-crypt.=0A= >>>>>=0A= >>>>> Thanks=0A= >>>>> Sudhakar=0A= >>>>=0A= >>>> Are you sure that the problem is only with dm-crypt? You haven't tried= =0A= >> all=0A= >>>> the existing block device drivers, have you?=0A= >>>=0A= >>> My question is, why dm-crypt worries about alignment requirement of=0A= >>> other layers? Is there anything that impacts dm-crypt if the segment=0A= >>> lengths are not aligned?(I believe this case is just not handled so far= =0A= >>=0A= >> Because the code is simpler if it assumes aligned buffers.=0A= > =0A= > Did you get a chance to review my changes? If you want more documentation= ,=0A= > improve the code, etc, let me know, I can do that if there is scope for t= hat.=0A= > =0A= >>=0A= >>> in dm-crypt and my patch addresses it). Should dm-crypt not just pass o= n=0A= >>> all those I/O requests to those respective layers to handle it which=0A= >>> will be more graceful?=0A= >>>=0A= >>> -Sudhakar=0A= >>=0A= >> So, suppose that we change dm-crypt to handle your workload - what are= =0A= >> you=0A= >> going to do with other block device drivers that assume aligned bio vect= or=0A= >> length? How will you find all the other drivers that need to be changed?= =0A= >>=0A= >> You are claiming that "every other component in the Linux IO path handle= s=0A= >> this case well except for dm-crypt", but this claim is wrong. There are= =0A= >> other driver that don't handle misaligned length as well.=0A= > =0A= > I should not have said, "every other component", I take that back, sorry.= How=0A= > about doing something like this in crypt_convert_block_skcipher():=0A= > =0A= > Add a check that looks at the alignment requirement of the low-level driv= er=0A= > and reject the I/O if it doesn't meet that requirement. This means, we st= ill=0A= > need to handle the case in this function where the low lever driver suppo= rt=0A= > unaligned buffer lengths, that means, my other changes in this function= =0A= > would still be needed.=0A= > =0A= > Is this acceptable to everyone?=0A= > =0A= > -Sudhakar=0A= > =0A= >>=0A= >> Mikulas=0A= >>=0A= >> --=0A= >> dm-devel mailing list=0A= >> dm-devel@redhat.com=0A= >> https://www.redhat.com/mailman/listinfo/dm-devel=0A= >>=0A= > =0A= =0A= =0A= -- =0A= Damien Le Moal=0A= Western Digital Research=0A= From mboxrd@z Thu Jan 1 00:00:00 1970 From: Damien Le Moal Subject: Re: [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices Date: Fri, 25 Sep 2020 01:09:52 +0000 Message-ID: References: <1600281606-1446-1-git-send-email-sudhakar.panneerselvam@oracle.com> <3be1ea32-b6a8-41ef-a9ba-ed691434d068@default> <20200924012732.GA10766@redhat.com> <20200924051419.GA16103@sol.localdomain> <252587bb-c0b7-47c9-a97b-91422f8f9c47@default> <7b6fdfd5-0160-4bcf-b7ed-d0e51553c678@default> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com Content-Language: en-US To: Sudhakar Panneerselvam , Mikulas Patocka Cc: "ssudhakarp@gmail.com" , Mike Snitzer , "dm-crypt@saout.de" , Eric Biggers , "dm-devel@redhat.com" , Ma , Martin Petersen , Shirley, Milan Broz , "agk@redhat.com" List-Id: dm-devel.ids On 2020/09/25 4:14, Sudhakar Panneerselvam wrote: >> >> On Thu, 24 Sep 2020, Sudhakar Panneerselvam wrote: >> >>>> By copying it to a temporary aligned buffer and issuing I/O on this >>>> buffer. >>> >>> I don't like this idea. Because, you need to allocate additional pages >>> for the entire I/O size(for the misaligned case, if you think through >> >> You can break the I/O to smaller pieces. You can use mempool for >> pre-allocation of the pages. > > Assuming we do this, how is this code simpler(based on your > comment below) than the fix in dm-crypt? In fact, this approach > would make the code change look bad in vhost, at the same time > having performance penalty. By doing this, we are just moving the > responsibility to other unrelated component. Because vhost is at the top of the block-io food chain. Fixing the unaligned segments there will ensure that it does not matter what device is under it. It will work. I am still baffled that the unaligned segments go through in the first place... Do we have something missing in the BIO code ? > >> >>> carefully, you will know why we have to allocate a temporary buffer that >>> is as big as the original IO) and on top of it, copying the buffer from >>> original to temporary buffer which is all unnecessary while it can >>> simply be fixed in dm-crypt without any of these additional overheads. >>> >>>> >>>>> Only other >>>>> possibility I see is to have windows fix it by always sending 512 byte >>>>> aligned buffer lengths, but going with my earlier point that every other >>>>> component in the Linux IO path handles this case well except for >>>>> dm-crypt, so it make more sense to fix it in dm-crypt. >>>>> >>>>> Thanks >>>>> Sudhakar >>>> >>>> Are you sure that the problem is only with dm-crypt? You haven't tried >> all >>>> the existing block device drivers, have you? >>> >>> My question is, why dm-crypt worries about alignment requirement of >>> other layers? Is there anything that impacts dm-crypt if the segment >>> lengths are not aligned?(I believe this case is just not handled so far >> >> Because the code is simpler if it assumes aligned buffers. > > Did you get a chance to review my changes? If you want more documentation, > improve the code, etc, let me know, I can do that if there is scope for that. > >> >>> in dm-crypt and my patch addresses it). Should dm-crypt not just pass on >>> all those I/O requests to those respective layers to handle it which >>> will be more graceful? >>> >>> -Sudhakar >> >> So, suppose that we change dm-crypt to handle your workload - what are >> you >> going to do with other block device drivers that assume aligned bio vector >> length? How will you find all the other drivers that need to be changed? >> >> You are claiming that "every other component in the Linux IO path handles >> this case well except for dm-crypt", but this claim is wrong. There are >> other driver that don't handle misaligned length as well. > > I should not have said, "every other component", I take that back, sorry. How > about doing something like this in crypt_convert_block_skcipher(): > > Add a check that looks at the alignment requirement of the low-level driver > and reject the I/O if it doesn't meet that requirement. This means, we still > need to handle the case in this function where the low lever driver support > unaligned buffer lengths, that means, my other changes in this function > would still be needed. > > Is this acceptable to everyone? > > -Sudhakar > >> >> Mikulas >> >> -- >> dm-devel mailing list >> dm-devel@redhat.com >> https://www.redhat.com/mailman/listinfo/dm-devel >> > -- Damien Le Moal Western Digital Research