* command emulation fix @ 2017-01-14 16:50 Christoph Hellwig 2017-01-14 16:50 ` [PATCH] libata: switch allocations for command emulation to GFP_ATOMIC Christoph Hellwig 2017-01-15 23:07 ` command emulation fix Tejun Heo 0 siblings, 2 replies; 13+ messages in thread From: Christoph Hellwig @ 2017-01-14 16:50 UTC (permalink / raw) To: tj; +Cc: linux-ide 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] libata: switch allocations for command emulation to GFP_ATOMIC 2017-01-14 16:50 command emulation fix Christoph Hellwig @ 2017-01-14 16:50 ` Christoph Hellwig 2017-01-15 23:07 ` command emulation fix Tejun Heo 1 sibling, 0 replies; 13+ messages in thread From: Christoph Hellwig @ 2017-01-14 16:50 UTC (permalink / raw) To: tj; +Cc: linux-ide ->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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: command emulation fix 2017-01-14 16:50 command emulation fix Christoph Hellwig 2017-01-14 16:50 ` [PATCH] libata: switch allocations for command emulation to GFP_ATOMIC Christoph Hellwig @ 2017-01-15 23:07 ` Tejun Heo 2017-01-16 15:21 ` Christoph Hellwig 1 sibling, 1 reply; 13+ messages in thread From: Tejun Heo @ 2017-01-15 23:07 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-ide 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: command emulation fix 2017-01-15 23:07 ` command emulation fix Tejun Heo @ 2017-01-16 15:21 ` Christoph Hellwig 2017-01-18 19:17 ` Tejun Heo 0 siblings, 1 reply; 13+ messages in thread From: Christoph Hellwig @ 2017-01-16 15:21 UTC (permalink / raw) To: Tejun Heo; +Cc: linux-ide 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: command emulation fix 2017-01-16 15:21 ` Christoph Hellwig @ 2017-01-18 19:17 ` Tejun Heo 2017-01-19 14:35 ` Christoph Hellwig 0 siblings, 1 reply; 13+ messages in thread From: Tejun Heo @ 2017-01-18 19:17 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-ide 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: command emulation fix 2017-01-18 19:17 ` Tejun Heo @ 2017-01-19 14:35 ` Christoph Hellwig 2017-01-19 20:23 ` Tejun Heo 0 siblings, 1 reply; 13+ messages in thread From: Christoph Hellwig @ 2017-01-19 14:35 UTC (permalink / raw) To: Tejun Heo; +Cc: Christoph Hellwig, linux-ide 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: command emulation fix 2017-01-19 14:35 ` Christoph Hellwig @ 2017-01-19 20:23 ` Tejun Heo 2017-01-19 20:27 ` Christoph Hellwig 0 siblings, 1 reply; 13+ messages in thread From: Tejun Heo @ 2017-01-19 20:23 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-ide 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: command emulation fix 2017-01-19 20:23 ` Tejun Heo @ 2017-01-19 20:27 ` Christoph Hellwig 2017-01-19 20:34 ` Tejun Heo 0 siblings, 1 reply; 13+ messages in thread From: Christoph Hellwig @ 2017-01-19 20:27 UTC (permalink / raw) To: Tejun Heo; +Cc: Christoph Hellwig, linux-ide 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: command emulation fix 2017-01-19 20:27 ` Christoph Hellwig @ 2017-01-19 20:34 ` Tejun Heo 2017-01-19 20:36 ` Tejun Heo 2017-01-19 20:40 ` Christoph Hellwig 0 siblings, 2 replies; 13+ messages in thread From: Tejun Heo @ 2017-01-19 20:34 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-ide 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: command emulation fix 2017-01-19 20:34 ` Tejun Heo @ 2017-01-19 20:36 ` Tejun Heo 2017-01-19 20:41 ` Christoph Hellwig 2017-01-19 20:40 ` Christoph Hellwig 1 sibling, 1 reply; 13+ messages in thread From: Tejun Heo @ 2017-01-19 20:36 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-ide 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: command emulation fix 2017-01-19 20:36 ` Tejun Heo @ 2017-01-19 20:41 ` Christoph Hellwig 2017-01-19 20:45 ` Tejun Heo 0 siblings, 1 reply; 13+ messages in thread From: Christoph Hellwig @ 2017-01-19 20:41 UTC (permalink / raw) To: Tejun Heo; +Cc: Christoph Hellwig, linux-ide 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: command emulation fix 2017-01-19 20:41 ` Christoph Hellwig @ 2017-01-19 20:45 ` Tejun Heo 0 siblings, 0 replies; 13+ messages in thread From: Tejun Heo @ 2017-01-19 20:45 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-ide 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: command emulation fix 2017-01-19 20:34 ` Tejun Heo 2017-01-19 20:36 ` Tejun Heo @ 2017-01-19 20:40 ` Christoph Hellwig 1 sibling, 0 replies; 13+ messages in thread From: Christoph Hellwig @ 2017-01-19 20:40 UTC (permalink / raw) To: Tejun Heo; +Cc: linux-ide, martin.petersen, linux-scsi 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-01-19 20:53 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-14 16:50 command emulation fix Christoph Hellwig 2017-01-14 16:50 ` [PATCH] libata: switch allocations for command emulation to GFP_ATOMIC Christoph Hellwig 2017-01-15 23:07 ` command emulation fix Tejun Heo 2017-01-16 15:21 ` Christoph Hellwig 2017-01-18 19:17 ` Tejun Heo 2017-01-19 14:35 ` Christoph Hellwig 2017-01-19 20:23 ` Tejun Heo 2017-01-19 20:27 ` Christoph Hellwig 2017-01-19 20:34 ` Tejun Heo 2017-01-19 20:36 ` Tejun Heo 2017-01-19 20:41 ` Christoph Hellwig 2017-01-19 20:45 ` Tejun Heo 2017-01-19 20:40 ` Christoph Hellwig
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.