command emulation fix

command emulation fix

[Christoph Hellwig]

Of course we can't just do a blind GFP_NOIO from ->queuecommand,
mea culpa.  For most of the commands GFP_ATOMIC should be absolutely
fine as they are only called from the probe path.  If you're
paranoid TRIM might want a sector-sized mempool, but other common
drivers like NVMe rely on plain GFP_ATOMIC allocations for those
as well.  Let me know, and I'll send a mempool patch on top of it.

[PATCH] libata: switch allocations for command emulation to GFP_ATOMIC

[Christoph Hellwig]

->queuecommand may be called under a spinlock or inside a critical section,
so we need to use GFP_ATOMIC for them.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Fixes a234f739 ("libata: switch to dynamic allocation instead of ata_scsi_rbuf")
---
 drivers/ata/libata-scsi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 4de273b..465fad1 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2074,7 +2074,7 @@ static void ata_scsi_rbuf_fill(struct ata_scsi_args *args,
 	struct scsi_cmnd *cmd = args->cmd;
 	u8 *buf;
 
-	buf = kzalloc(ATA_SCSI_RBUF_SIZE, GFP_NOIO);
+	buf = kzalloc(ATA_SCSI_RBUF_SIZE, GFP_ATOMIC);
 	if (!buf) {
 		ata_scsi_set_sense(args->dev, cmd, NOT_READY, 0x08, 0);
 		return;
@@ -3325,7 +3325,7 @@ static ssize_t ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 trmax,
 	__le64 *buf;
 	u32 i = 0;
 
-	buf = kzalloc(cmd->device->sector_size, GFP_NOFS);
+	buf = kzalloc(cmd->device->sector_size, GFP_ATOMIC);
 	if (!buf)
 		return -ENOMEM;
 
@@ -3362,7 +3362,7 @@ static ssize_t ata_format_sct_write_same(struct scsi_cmnd *cmd, u64 lba,
 	size_t r;
 	u16 *buf;
 
-	buf = kzalloc(cmd->device->sector_size, GFP_NOIO);
+	buf = kzalloc(cmd->device->sector_size, GFP_ATOMIC);
 	if (!buf)
 		return -ENOMEM;
 
-- 
2.1.4

Re: command emulation fix

[Tejun Heo]

Hello, Christoph.

On Sat, Jan 14, 2017 at 05:50:01PM +0100, Christoph Hellwig wrote:
> Of course we can't just do a blind GFP_NOIO from ->queuecommand,
> mea culpa.  For most of the commands GFP_ATOMIC should be absolutely
> fine as they are only called from the probe path.  If you're
> paranoid TRIM might want a sector-sized mempool, but other common
> drivers like NVMe rely on plain GFP_ATOMIC allocations for those
> as well.  Let me know, and I'll send a mempool patch on top of it.

Ugh... I don't know.  What we had previously is always guaranteed to
work.  I'm not really liking the fact that we're adding a possibility
of failure here.  Even if we do mempool, we would still have to
protect it with a spinlock as mempool only guarantees one allocation
at a time.  Until we have a better solution, can we please revert back
to where we were at least for the buffers needed from atomic context?

Thanks.

-- 
tejun

Re: command emulation fix

[Christoph Hellwig]

On Sun, Jan 15, 2017 at 06:07:23PM -0500, Tejun Heo wrote:
> Ugh... I don't know.  What we had previously is always guaranteed to
> work.  I'm not really liking the fact that we're adding a possibility
> of failure here.  Even if we do mempool, we would still have to
> protect it with a spinlock as mempool only guarantees one allocation
> at a time.  Until we have a better solution, can we please revert back
> to where we were at least for the buffers needed from atomic context?

In that case you'll need to drop all but the first patch from the series
as ->queuecommand can always be called from atomic context.

Re: command emulation fix

[Tejun Heo]

Hello,

On Mon, Jan 16, 2017 at 04:21:20PM +0100, Christoph Hellwig wrote:
> On Sun, Jan 15, 2017 at 06:07:23PM -0500, Tejun Heo wrote:
> > Ugh... I don't know.  What we had previously is always guaranteed to
> > work.  I'm not really liking the fact that we're adding a possibility
> > of failure here.  Even if we do mempool, we would still have to
> > protect it with a spinlock as mempool only guarantees one allocation
> > at a time.  Until we have a better solution, can we please revert back
> > to where we were at least for the buffers needed from atomic context?
> 
> In that case you'll need to drop all but the first patch from the series
> as ->queuecommand can always be called from atomic context.

I reverted just the last patch.  The rest should be fine, right?

Thanks.

-- 
tejun

Re: command emulation fix

[Christoph Hellwig]

On Wed, Jan 18, 2017 at 11:17:50AM -0800, Tejun Heo wrote:
> I reverted just the last patch.  The rest should be fine, right?

Yes.  And personally I think the last one is also fine, but we
seem to disagree.

Re: command emulation fix

[Tejun Heo]

Hello, Christoph.

On Thu, Jan 19, 2017 at 03:35:51PM +0100, Christoph Hellwig wrote:
> On Wed, Jan 18, 2017 at 11:17:50AM -0800, Tejun Heo wrote:
> > I reverted just the last patch.  The rest should be fine, right?
> 
> Yes.  And personally I think the last one is also fine, but we
> seem to disagree.

Yeah, it is probably fine but I just feel weird about changing the
code to introduce a possible failure case.  Let's leave it as-is for
now.

Thanks.

-- 
tejun

Re: command emulation fix

[Christoph Hellwig]

On Thu, Jan 19, 2017 at 03:23:16PM -0500, Tejun Heo wrote:
> Yeah, it is probably fine but I just feel weird about changing the
> code to introduce a possible failure case.  Let's leave it as-is for
> now.

We can move to a small mempool instead.  If we ever want discards
(aka TRIM) to perform nicely on ATA a global static buffer isn't
going to cut it.  I've spent some time to optimize it for NVMe so
that it's usable which I plan to submit soon.  I've not looked ATA
in that respect yet, but it's probably going to be an issue.

Re: command emulation fix

[Tejun Heo]

Hello, Christoph.

On Thu, Jan 19, 2017 at 09:27:25PM +0100, Christoph Hellwig wrote:
> We can move to a small mempool instead.  If we ever want discards
> (aka TRIM) to perform nicely on ATA a global static buffer isn't
> going to cut it.  I've spent some time to optimize it for NVMe so
> that it's usable which I plan to submit soon.  I've not looked ATA
> in that respect yet, but it's probably going to be an issue.

So, unless we allocate one mempool per port, we're gonna have to
synchronize around its use anyway.  mempool can't guarantee allocation
to multiple users at the same time.  If this is something which
affects scalability, I'm completley fine with making it per-port (or
device).  Each ata_port carries 512 byte buffer anyway.  Maybe we can
reuse that?

Thanks.

-- 
tejun

Re: command emulation fix

[Tejun Heo]

On Thu, Jan 19, 2017 at 03:34:58PM -0500, Tejun Heo wrote:
> Hello, Christoph.
> 
> On Thu, Jan 19, 2017 at 09:27:25PM +0100, Christoph Hellwig wrote:
> > We can move to a small mempool instead.  If we ever want discards
> > (aka TRIM) to perform nicely on ATA a global static buffer isn't
> > going to cut it.  I've spent some time to optimize it for NVMe so
> > that it's usable which I plan to submit soon.  I've not looked ATA
> > in that respect yet, but it's probably going to be an issue.
> 
> So, unless we allocate one mempool per port, we're gonna have to
> synchronize around its use anyway.  mempool can't guarantee allocation
> to multiple users at the same time.  If this is something which
> affects scalability, I'm completley fine with making it per-port (or
> device).  Each ata_port carries 512 byte buffer anyway.  Maybe we can
> reuse that?

But do you actually expect there to be scalability issues around this
buffer?  Its uses are extremely short and libata operations are pretty
strongly synchronized to begin with.

Thanks.

-- 
tejun

Re: command emulation fix

[Christoph Hellwig]

On Thu, Jan 19, 2017 at 03:34:58PM -0500, Tejun Heo wrote:
> So, unless we allocate one mempool per port, we're gonna have to
> synchronize around its use anyway.  mempool can't guarantee allocation
> to multiple users at the same time.  If this is something which
> affects scalability, I'm completley fine with making it per-port (or
> device).  Each ata_port carries 512 byte buffer anyway.  Maybe we can
> reuse that?

I'll play around with these things a bit more.  For the slow
path ops I think I can get away without any dynamic allocation
at all - just use small on-stack buffers.

For DSM / Write Same we rewrite into the buffer the SCSI layer
provided us.  This is a bit of an issue anyway as this might
modify user data that is not expected to be rewritten or even
mapped read-only.  Maybe we need to kill off that emulation
entirely and just have ATA DSM using the ATA_16 CDB as another
provisioning option in sd.c.  While that is a bit of a layering
violation it would solve a lot of problems with the way TRIM
is currently implemented.

Re: command emulation fix

[Christoph Hellwig]

On Thu, Jan 19, 2017 at 03:36:46PM -0500, Tejun Heo wrote:
> But do you actually expect there to be scalability issues around this
> buffer?  Its uses are extremely short and libata operations are pretty
> strongly synchronized to begin with.

Mostly for setups using JBODs with massive numbers of SATA disks.

This would mostly not be ahci or a real ATA driver but libsas calling
into libata for the translation.

Re: command emulation fix

[Tejun Heo]

Hello,

On Thu, Jan 19, 2017 at 09:41:35PM +0100, Christoph Hellwig wrote:
> On Thu, Jan 19, 2017 at 03:36:46PM -0500, Tejun Heo wrote:
> > But do you actually expect there to be scalability issues around this
> > buffer?  Its uses are extremely short and libata operations are pretty
> > strongly synchronized to begin with.
> 
> Mostly for setups using JBODs with massive numbers of SATA disks.
> 
> This would mostly not be ahci or a real ATA driver but libsas calling
> into libata for the translation.

Ah, understood.

Thanks for the explanation.

-- 
tejun