Prev: [PATCH 1/9] mm: add generic adaptive large memory allocation APIs
Next: ramzswap: Remove backing swap support
From: Christoph Hellwig on 13 May 2010 11:20
On Wed, May 12, 2010 at 04:40:53PM -0400, Josef Bacik wrote:
> V1->V2
> -Use __blockdev_direct_IO instead of helper
> -Use KM_IRQ0 for kmap instead of KM_USER0

I still don't think this is correct. The completion can come from
softirq and hardirq context, and possibly even normal process context.
You either need to check for all these, or you need to use the generic
complete in user context helper btrfs has available for other types
of I/O.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Josef Bacik on 13 May 2010 11:30
On Thu, May 13, 2010 at 11:14:30AM -0400, Christoph Hellwig wrote:
> On Wed, May 12, 2010 at 04:40:53PM -0400, Josef Bacik wrote:
> > V1->V2
> > -Use __blockdev_direct_IO instead of helper
> > -Use KM_IRQ0 for kmap instead of KM_USER0
>
> I still don't think this is correct. The completion can come from
> softirq and hardirq context, and possibly even normal process context.
> You either need to check for all these, or you need to use the generic
> complete in user context helper btrfs has available for other types
> of I/O.
>

AIO's aio_complete does kmap with KM_IRQ0/1 and it gets called in the same
context as the btrfs completion handler, so if it's ok for aio_complete it
should be ok for btrfs right? Thanks,

Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Christoph Hellwig on 13 May 2010 11:30
On Thu, May 13, 2010 at 11:31:45AM -0400, Josef Bacik wrote:
> AIO's aio_complete does kmap with KM_IRQ0/1 and it gets called in the same
> context as the btrfs completion handler, so if it's ok for aio_complete it
> should be ok for btrfs right? Thanks,

aio_complete does a spin_lock_irqsave before that, which disables
interrupts on the local CPU.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Josef Bacik on 13 May 2010 11:40
On Thu, May 13, 2010 at 11:26:39AM -0400, Christoph Hellwig wrote:
> On Thu, May 13, 2010 at 11:31:45AM -0400, Josef Bacik wrote:
> > AIO's aio_complete does kmap with KM_IRQ0/1 and it gets called in the same
> > context as the btrfs completion handler, so if it's ok for aio_complete it
> > should be ok for btrfs right? Thanks,
>
> aio_complete does a spin_lock_irqsave before that, which disables
> interrupts on the local CPU.
>

Ok so looking at debug_kmap_atomic it seems that KM_IRQ0 is ok to use in all
cases, in_irq(), !irqs_disabled() and !in_interrupt(), so I should be safe. If
you still don't like it, would something like this be acceptable?

if (in_interrupt() || irqs_disabled()) {
if (in_irq())
type = KM_IRQ0;
else if (!irqs_disabled())
type = KM_SOFTIRQ0;
else
type = KM_IRQ0;
} else {
type = KM_USER0;
}

Hrm, you'd think somebody would have had this problem before and already come up
with a nice helper, oh well. Thanks,

Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 13 May 2010 11:50
On Thu, 13 May 2010 11:31:45 -0400 Josef Bacik <josef(a)redhat.com> wrote:

> On Thu, May 13, 2010 at 11:14:30AM -0400, Christoph Hellwig wrote:
> > On Wed, May 12, 2010 at 04:40:53PM -0400, Josef Bacik wrote:
> > > V1->V2
> > > -Use __blockdev_direct_IO instead of helper
> > > -Use KM_IRQ0 for kmap instead of KM_USER0
> >
> > I still don't think this is correct. The completion can come from
> > softirq and hardirq context, and possibly even normal process context.
> > You either need to check for all these, or you need to use the generic
> > complete in user context helper btrfs has available for other types
> > of I/O.
> >
>
> AIO's aio_complete does kmap with KM_IRQ0/1 and it gets called in the same
> context as the btrfs completion handler, so if it's ok for aio_complete it
> should be ok for btrfs right? Thanks,
>

Using KM_USERx from irq context is a horrid, horrid bug.

Using KM_IRQx with local interrupts enabled is a ditto.

It's OK to use KM_IRQx from process context as long as local interrupts
are disabled.

kmap slots are just per-cpu variables. Each one has a particular usage
convention: process-context-only, must-be-irq-safe, etc.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: [PATCH 1/9] mm: add generic adaptive large memory allocation APIs
Next: ramzswap: Remove backing swap support