[PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Ming Lei]

XFS uses kmalloc() to allocate sector sized IO buffer.

Turns out buffer allocated via kmalloc(sector sized) can't
be guaranteed to be 512 byte aligned, and actually slab only provides
ARCH_KMALLOC_MINALIGN alignment, even though it is observed
that the sector size allocation is often 512 byte aligned. When
KASAN or other memory debug options are enabled, the allocated
buffer becomes not aliged with 512 byte any more.

This unalgined IO buffer causes at least two issues:

1) some storage controller requires IO buffer to be 512 byte aligned,
and data corruption is observed

2) loop/dio requires the IO buffer to be logical block size aligned,
and loop's default logcial block size is 512 byte, then one xfs image
can't be mounted via loop/dio any more.

Use page_frag_alloc() to allocate the sector sized buffer, then the
above issue can be fixed because offset_in_page of allocated buffer
is always sector aligned.

Not see any regression with this patch on xfstests.

Cc: Jens Axboe <axboe@kernel.dk>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Darrick J. Wong <darrick.wong@oracle.com>
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com>
Cc: Aaron Lu <aaron.lu@intel.com>
Cc: Christopher Lameter <cl@linux.com>
Cc: Linux FS Devel <linux-fsdevel@vger.kernel.org>
Cc: linux-mm@kvack.org
Cc: linux-block@vger.kernel.org
Link: https://marc.info/?t=153734857500004&r=1&w=2
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 fs/xfs/xfs_buf.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 4f5f2ff3f70f..92b8cdf5e51c 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -340,12 +340,27 @@ xfs_buf_free(
 			__free_page(page);
 		}
 	} else if (bp->b_flags & _XBF_KMEM)
-		kmem_free(bp->b_addr);
+		page_frag_free(bp->b_addr);
 	_xfs_buf_free_pages(bp);
 	xfs_buf_free_maps(bp);
 	kmem_zone_free(xfs_buf_zone, bp);
 }
 
+static DEFINE_PER_CPU(struct page_frag_cache, xfs_frag_cache);
+
+static void *xfs_alloc_frag(int size)
+{
+	struct page_frag_cache *nc;
+	void *data;
+
+	preempt_disable();
+	nc = this_cpu_ptr(&xfs_frag_cache);
+	data = page_frag_alloc(nc, size, GFP_ATOMIC);
+	preempt_enable();
+
+	return data;
+}
+
 /*
  * Allocates all the pages for buffer in question and builds it's page list.
  */
@@ -368,7 +383,7 @@ xfs_buf_allocate_memory(
 	 */
 	size = BBTOB(bp->b_length);
 	if (size < PAGE_SIZE) {
-		bp->b_addr = kmem_alloc(size, KM_NOFS);
+		bp->b_addr = xfs_alloc_frag(size);
 		if (!bp->b_addr) {
 			/* low memory - use alloc_page loop instead */
 			goto use_alloc_page;
@@ -377,7 +392,7 @@ xfs_buf_allocate_memory(
 		if (((unsigned long)(bp->b_addr + size - 1) & PAGE_MASK) !=
 		    ((unsigned long)bp->b_addr & PAGE_MASK)) {
 			/* b_addr spans two pages - use alloc_page instead */
-			kmem_free(bp->b_addr);
+			page_frag_free(bp->b_addr);
 			bp->b_addr = NULL;
 			goto use_alloc_page;
 		}
-- 
2.9.5

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Dave Chinner]

On Mon, Feb 25, 2019 at 12:09:04PM +0800, Ming Lei wrote:
> XFS uses kmalloc() to allocate sector sized IO buffer.
....
> Use page_frag_alloc() to allocate the sector sized buffer, then the
> above issue can be fixed because offset_in_page of allocated buffer
> is always sector aligned.

Didn't we already reject this approach because page frags cannot be
reused and that pages allocated to the frag pool are pinned in
memory until all fragments allocated on the page have been freed?

i.e. when we consider 64k page machines and 4k block sizes (i.e.
default config), every single metadata allocation is a sub-page
allocation and so will use this new page frag mechanism. IOWs, it
will result in fragmenting memory severely and typical memory
reclaim not being able to fix it because the metadata that pins each
page is largely unreclaimable...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Ming Lei]

On Mon, Feb 25, 2019 at 03:36:48PM +1100, Dave Chinner wrote:
> On Mon, Feb 25, 2019 at 12:09:04PM +0800, Ming Lei wrote:
> > XFS uses kmalloc() to allocate sector sized IO buffer.
> ....
> > Use page_frag_alloc() to allocate the sector sized buffer, then the
> > above issue can be fixed because offset_in_page of allocated buffer
> > is always sector aligned.
> 
> Didn't we already reject this approach because page frags cannot be

I remembered there is this kind of issue mentioned, but just not found
the details, so post out the patch for restarting the discussion.

> reused and that pages allocated to the frag pool are pinned in
> memory until all fragments allocated on the page have been freed?

Yes, that is one problem. But if one page is consumed, sooner or later,
all fragments will be freed, then the page becomes available again.

> 
> i.e. when we consider 64k page machines and 4k block sizes (i.e.
> default config), every single metadata allocation is a sub-page
> allocation and so will use this new page frag mechanism. IOWs, it
> will result in fragmenting memory severely and typical memory
> reclaim not being able to fix it because the metadata that pins each
> page is largely unreclaimable...

It can be an issue in case of IO timeout & retry.


Thanks,
Ming

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Ming Lei]

On Mon, Feb 25, 2019 at 04:46:25PM +0800, Ming Lei wrote:
> On Mon, Feb 25, 2019 at 03:36:48PM +1100, Dave Chinner wrote:
> > On Mon, Feb 25, 2019 at 12:09:04PM +0800, Ming Lei wrote:
> > > XFS uses kmalloc() to allocate sector sized IO buffer.
> > ....
> > > Use page_frag_alloc() to allocate the sector sized buffer, then the
> > > above issue can be fixed because offset_in_page of allocated buffer
> > > is always sector aligned.
> > 
> > Didn't we already reject this approach because page frags cannot be
> 
> I remembered there is this kind of issue mentioned, but just not found
> the details, so post out the patch for restarting the discussion.
> 
> > reused and that pages allocated to the frag pool are pinned in
> > memory until all fragments allocated on the page have been freed?
> 
> Yes, that is one problem. But if one page is consumed, sooner or later,
> all fragments will be freed, then the page becomes available again.
> 
> > 
> > i.e. when we consider 64k page machines and 4k block sizes (i.e.
> > default config), every single metadata allocation is a sub-page
> > allocation and so will use this new page frag mechanism. IOWs, it
> > will result in fragmenting memory severely and typical memory
> > reclaim not being able to fix it because the metadata that pins each
> > page is largely unreclaimable...
> 
> It can be an issue in case of IO timeout & retry.

The worst case is still not worse than allocating single page for sub-page
IO, which should be used on other file systems under the same situation,
I guess.

thanks,
Ming

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Dave Chinner]

On Mon, Feb 25, 2019 at 04:46:25PM +0800, Ming Lei wrote:
> On Mon, Feb 25, 2019 at 03:36:48PM +1100, Dave Chinner wrote:
> > On Mon, Feb 25, 2019 at 12:09:04PM +0800, Ming Lei wrote:
> > > XFS uses kmalloc() to allocate sector sized IO buffer.
> > ....
> > > Use page_frag_alloc() to allocate the sector sized buffer, then the
> > > above issue can be fixed because offset_in_page of allocated buffer
> > > is always sector aligned.
> > 
> > Didn't we already reject this approach because page frags cannot be
> 
> I remembered there is this kind of issue mentioned, but just not found
> the details, so post out the patch for restarting the discussion.

As previously discussed, the only solution that fits all use cases
we have to support are a slab caches that do not break object
alignment when slab debug options are turned on.

> > reused and that pages allocated to the frag pool are pinned in
> > memory until all fragments allocated on the page have been freed?
> 
> Yes, that is one problem. But if one page is consumed, sooner or later,
> all fragments will be freed, then the page becomes available again.

Ah, no, your assumption about how metadata caching in XFS works is
flawed. Some metadata ends up being cached for the life of the
filesystem because it is so frequently referenced it never gets
reclaimed. AG headers, btree root blocks, etc.  And the XFS metadata
cache hangs on to such metadata even under extreme memory pressure
because if we reclaim it then any filesystem operation will need to
reallocate that memory to clean dirty pages and that is the very
last thing we want to do under extreme memory pressure conditions.

If allocation cannot reuse holes in pages (i.e. works as a proper
slab cache) then we are going to blow out the amount of memory that
the XFS metadata cache uses very badly on filesystems where block
size != page size. 

> > i.e. when we consider 64k page machines and 4k block sizes (i.e.
> > default config), every single metadata allocation is a sub-page
> > allocation and so will use this new page frag mechanism. IOWs, it
> > will result in fragmenting memory severely and typical memory
> > reclaim not being able to fix it because the metadata that pins each
> > page is largely unreclaimable...
> 
> It can be an issue in case of IO timeout & retry.

This makes no sense to me. Exactly how does filesystem memory
allocation affect IO timeouts and any retries the filesystem might
issue?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Vlastimil Babka]

On 2/25/19 5:36 AM, Dave Chinner wrote:
> On Mon, Feb 25, 2019 at 12:09:04PM +0800, Ming Lei wrote:
>> XFS uses kmalloc() to allocate sector sized IO buffer.
> ....
>> Use page_frag_alloc() to allocate the sector sized buffer, then the
>> above issue can be fixed because offset_in_page of allocated buffer
>> is always sector aligned.
> 
> Didn't we already reject this approach because page frags cannot be
> reused and that pages allocated to the frag pool are pinned in
> memory until all fragments allocated on the page have been freed?

I don't know if you did, but it's certainly true., Also I don't think
there's any specified alignment guarantee for page_frag_alloc().

What about kmem_cache_create() with align parameter? That *should* be
guaranteed regardless of whatever debugging is enabled - if not, I would
consider it a bug.

> i.e. when we consider 64k page machines and 4k block sizes (i.e.
> default config), every single metadata allocation is a sub-page
> allocation and so will use this new page frag mechanism. IOWs, it
> will result in fragmenting memory severely and typical memory
> reclaim not being able to fix it because the metadata that pins each
> page is largely unreclaimable...
> 
> Cheers,
> 
> Dave.
> 

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Dave Chinner]

On Mon, Feb 25, 2019 at 02:15:59PM +0100, Vlastimil Babka wrote:
> On 2/25/19 5:36 AM, Dave Chinner wrote:
> > On Mon, Feb 25, 2019 at 12:09:04PM +0800, Ming Lei wrote:
> >> XFS uses kmalloc() to allocate sector sized IO buffer.
> > ....
> >> Use page_frag_alloc() to allocate the sector sized buffer, then the
> >> above issue can be fixed because offset_in_page of allocated buffer
> >> is always sector aligned.
> > 
> > Didn't we already reject this approach because page frags cannot be
> > reused and that pages allocated to the frag pool are pinned in
> > memory until all fragments allocated on the page have been freed?
> 
> I don't know if you did, but it's certainly true., Also I don't think
> there's any specified alignment guarantee for page_frag_alloc().

We did, and the alignment guarantee would have come from all
fragments having an aligned size.

> What about kmem_cache_create() with align parameter? That *should* be
> guaranteed regardless of whatever debugging is enabled - if not, I would
> consider it a bug.

Yup, that's pretty much what was decided. The sticking point was
whether is should be block layer infrastructure (because the actual
memory buffer alignment is a block/device driver requirement not
visible to the filesystem) or whether "sector size alignement is
good enough for everyone".

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Ming Lei]

On Tue, Feb 26, 2019 at 07:26:30AM +1100, Dave Chinner wrote:
> On Mon, Feb 25, 2019 at 02:15:59PM +0100, Vlastimil Babka wrote:
> > On 2/25/19 5:36 AM, Dave Chinner wrote:
> > > On Mon, Feb 25, 2019 at 12:09:04PM +0800, Ming Lei wrote:
> > >> XFS uses kmalloc() to allocate sector sized IO buffer.
> > > ....
> > >> Use page_frag_alloc() to allocate the sector sized buffer, then the
> > >> above issue can be fixed because offset_in_page of allocated buffer
> > >> is always sector aligned.
> > > 
> > > Didn't we already reject this approach because page frags cannot be
> > > reused and that pages allocated to the frag pool are pinned in
> > > memory until all fragments allocated on the page have been freed?
> > 
> > I don't know if you did, but it's certainly true., Also I don't think
> > there's any specified alignment guarantee for page_frag_alloc().
> 
> We did, and the alignment guarantee would have come from all
> fragments having an aligned size.
> 
> > What about kmem_cache_create() with align parameter? That *should* be
> > guaranteed regardless of whatever debugging is enabled - if not, I would
> > consider it a bug.
> 
> Yup, that's pretty much what was decided. The sticking point was
> whether is should be block layer infrastructure (because the actual
> memory buffer alignment is a block/device driver requirement not
> visible to the filesystem) or whether "sector size alignement is
> good enough for everyone".

OK, looks I miss the long life time of meta data caching, then let's
discuss the slab approach.

Looks one single slab cache doesn't work, given the size may be 512 * N
(1 <= N < PAGE_SIZE/512), that is basically what I posted the first
time.

https://marc.info/?t=153986884900007&r=1&w=2
https://marc.info/?t=153986885100001&r=1&w=2

Or what is the exact size of sub-page IO in xfs most of time? For
example, if 99% times falls in 512 byte allocation, maybe it is enough
to just maintain one 512byte slab.

Thanks,
Ming

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Dave Chinner]

On Tue, Feb 26, 2019 at 10:22:50AM +0800, Ming Lei wrote:
> On Tue, Feb 26, 2019 at 07:26:30AM +1100, Dave Chinner wrote:
> > On Mon, Feb 25, 2019 at 02:15:59PM +0100, Vlastimil Babka wrote:
> > > On 2/25/19 5:36 AM, Dave Chinner wrote:
> > > > On Mon, Feb 25, 2019 at 12:09:04PM +0800, Ming Lei wrote:
> > > >> XFS uses kmalloc() to allocate sector sized IO buffer.
> > > > ....
> > > >> Use page_frag_alloc() to allocate the sector sized buffer, then the
> > > >> above issue can be fixed because offset_in_page of allocated buffer
> > > >> is always sector aligned.
> > > > 
> > > > Didn't we already reject this approach because page frags cannot be
> > > > reused and that pages allocated to the frag pool are pinned in
> > > > memory until all fragments allocated on the page have been freed?
> > > 
> > > I don't know if you did, but it's certainly true., Also I don't think
> > > there's any specified alignment guarantee for page_frag_alloc().
> > 
> > We did, and the alignment guarantee would have come from all
> > fragments having an aligned size.
> > 
> > > What about kmem_cache_create() with align parameter? That *should* be
> > > guaranteed regardless of whatever debugging is enabled - if not, I would
> > > consider it a bug.
> > 
> > Yup, that's pretty much what was decided. The sticking point was
> > whether is should be block layer infrastructure (because the actual
> > memory buffer alignment is a block/device driver requirement not
> > visible to the filesystem) or whether "sector size alignement is
> > good enough for everyone".
> 
> OK, looks I miss the long life time of meta data caching, then let's
> discuss the slab approach.
> 
> Looks one single slab cache doesn't work, given the size may be 512 * N
> (1 <= N < PAGE_SIZE/512), that is basically what I posted the first
> time.
> 
> https://marc.info/?t=153986884900007&r=1&w=2
> https://marc.info/?t=153986885100001&r=1&w=2
> 
> Or what is the exact size of sub-page IO in xfs most of time? For

Determined by mkfs parameters. Any power of 2 between 512 bytes and
64kB needs to be supported. e.g:

# mkfs.xfs -s size=512 -b size=1k -i size=2k -n size=8k ....

will have metadata that is sector sized (512 bytes), filesystem
block sized (1k), directory block sized (8k) and inode cluster sized
(32k), and will use all of them in large quantities.

> example, if 99% times falls in 512 byte allocation, maybe it is enough
> to just maintain one 512byte slab.

It is not. On a 64k page size machine, we use sub page slabs for
metadata blocks of 2^N bytes where 9 <= N <= 15..

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Matthew Wilcox]

On Tue, Feb 26, 2019 at 02:02:14PM +1100, Dave Chinner wrote:
> > Or what is the exact size of sub-page IO in xfs most of time? For
> 
> Determined by mkfs parameters. Any power of 2 between 512 bytes and
> 64kB needs to be supported. e.g:
> 
> # mkfs.xfs -s size=512 -b size=1k -i size=2k -n size=8k ....
> 
> will have metadata that is sector sized (512 bytes), filesystem
> block sized (1k), directory block sized (8k) and inode cluster sized
> (32k), and will use all of them in large quantities.

If XFS is going to use each of these in large quantities, then it doesn't
seem unreasonable for XFS to create a slab for each type of metadata?

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Dave Chinner]

On Mon, Feb 25, 2019 at 07:27:37PM -0800, Matthew Wilcox wrote:
> On Tue, Feb 26, 2019 at 02:02:14PM +1100, Dave Chinner wrote:
> > > Or what is the exact size of sub-page IO in xfs most of time? For
> > 
> > Determined by mkfs parameters. Any power of 2 between 512 bytes and
> > 64kB needs to be supported. e.g:
> > 
> > # mkfs.xfs -s size=512 -b size=1k -i size=2k -n size=8k ....
> > 
> > will have metadata that is sector sized (512 bytes), filesystem
> > block sized (1k), directory block sized (8k) and inode cluster sized
> > (32k), and will use all of them in large quantities.
> 
> If XFS is going to use each of these in large quantities, then it doesn't
> seem unreasonable for XFS to create a slab for each type of metadata?


Well, that is the question, isn't it? How many other filesystems
will want to make similar "don't use entire pages just for 4k of
metadata" optimisations as 64k page size machines become more
common? There are others that have the same "use slab for sector
aligned IO" which will fall foul of the same problem that has been
reported for XFS....

If nobody else cares/wants it, then it can be XFS only. But it's
only fair we address the "will it be useful to others" question
first.....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Ming Lei]

On Tue, Feb 26, 2019 at 03:58:26PM +1100, Dave Chinner wrote:
> On Mon, Feb 25, 2019 at 07:27:37PM -0800, Matthew Wilcox wrote:
> > On Tue, Feb 26, 2019 at 02:02:14PM +1100, Dave Chinner wrote:
> > > > Or what is the exact size of sub-page IO in xfs most of time? For
> > > 
> > > Determined by mkfs parameters. Any power of 2 between 512 bytes and
> > > 64kB needs to be supported. e.g:
> > > 
> > > # mkfs.xfs -s size=512 -b size=1k -i size=2k -n size=8k ....
> > > 
> > > will have metadata that is sector sized (512 bytes), filesystem
> > > block sized (1k), directory block sized (8k) and inode cluster sized
> > > (32k), and will use all of them in large quantities.
> > 
> > If XFS is going to use each of these in large quantities, then it doesn't
> > seem unreasonable for XFS to create a slab for each type of metadata?
> 
> 
> Well, that is the question, isn't it? How many other filesystems
> will want to make similar "don't use entire pages just for 4k of
> metadata" optimisations as 64k page size machines become more
> common? There are others that have the same "use slab for sector
> aligned IO" which will fall foul of the same problem that has been
> reported for XFS....
> 
> If nobody else cares/wants it, then it can be XFS only. But it's
> only fair we address the "will it be useful to others" question
> first.....

This kind of slab cache should have been global, just like interface of
kmalloc(size).

However, the alignment requirement depends on block device's block size,
then it becomes hard to implement as genera interface, for example:

	block size: 512, 1024, 2048, 4096
	slab size: 512*N, 0 < N < PAGE_SIZE/512

For 4k page size, 28(7*4) slabs need to be created, and 64k page size
needs to create 127*4 slabs.

But, specific file system may only use some of them, and it depends
on meta data size.

Thanks,
Ming

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Vlastimil Babka]

On 2/26/19 10:33 AM, Ming Lei wrote:
> On Tue, Feb 26, 2019 at 03:58:26PM +1100, Dave Chinner wrote:
>> On Mon, Feb 25, 2019 at 07:27:37PM -0800, Matthew Wilcox wrote:
>>> On Tue, Feb 26, 2019 at 02:02:14PM +1100, Dave Chinner wrote:
>>>>> Or what is the exact size of sub-page IO in xfs most of time? For
>>>>
>>>> Determined by mkfs parameters. Any power of 2 between 512 bytes and
>>>> 64kB needs to be supported. e.g:
>>>>
>>>> # mkfs.xfs -s size=512 -b size=1k -i size=2k -n size=8k ....
>>>>
>>>> will have metadata that is sector sized (512 bytes), filesystem
>>>> block sized (1k), directory block sized (8k) and inode cluster sized
>>>> (32k), and will use all of them in large quantities.
>>>
>>> If XFS is going to use each of these in large quantities, then it doesn't
>>> seem unreasonable for XFS to create a slab for each type of metadata?
>>
>>
>> Well, that is the question, isn't it? How many other filesystems
>> will want to make similar "don't use entire pages just for 4k of
>> metadata" optimisations as 64k page size machines become more
>> common? There are others that have the same "use slab for sector
>> aligned IO" which will fall foul of the same problem that has been
>> reported for XFS....
>>
>> If nobody else cares/wants it, then it can be XFS only. But it's
>> only fair we address the "will it be useful to others" question
>> first.....
> 
> This kind of slab cache should have been global, just like interface of
> kmalloc(size).
> 
> However, the alignment requirement depends on block device's block size,
> then it becomes hard to implement as genera interface, for example:
> 
> 	block size: 512, 1024, 2048, 4096
> 	slab size: 512*N, 0 < N < PAGE_SIZE/512
> 
> For 4k page size, 28(7*4) slabs need to be created, and 64k page size
> needs to create 127*4 slabs.
>

Where does the '*4' multiplier come from?

So I wonder how hard would it actually be (+CC slab maintainers) to just
guarantee generic kmalloc() alignment for power-of-two sizes. If we can
do that for kmem_cache_create() then the code should be already there.
AFAIK the alignment happens anyway (albeit not guaranteed) in the
non-debug cases, and if guaranteeing alignment for certain debugging
configurations (that need some space before the object) means larger
memory overhead, then the cost should still be bearable since its
non-standard configuration where the point is to catch bug and not have
peak performance?

> But, specific file system may only use some of them, and it depends
> on meta data size.
> 
> Thanks,
> Ming
> 

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Ming Lei]

On Tue, Feb 26, 2019 at 6:07 PM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 2/26/19 10:33 AM, Ming Lei wrote:
> > On Tue, Feb 26, 2019 at 03:58:26PM +1100, Dave Chinner wrote:
> >> On Mon, Feb 25, 2019 at 07:27:37PM -0800, Matthew Wilcox wrote:
> >>> On Tue, Feb 26, 2019 at 02:02:14PM +1100, Dave Chinner wrote:
> >>>>> Or what is the exact size of sub-page IO in xfs most of time? For
> >>>>
> >>>> Determined by mkfs parameters. Any power of 2 between 512 bytes and
> >>>> 64kB needs to be supported. e.g:
> >>>>
> >>>> # mkfs.xfs -s size=512 -b size=1k -i size=2k -n size=8k ....
> >>>>
> >>>> will have metadata that is sector sized (512 bytes), filesystem
> >>>> block sized (1k), directory block sized (8k) and inode cluster sized
> >>>> (32k), and will use all of them in large quantities.
> >>>
> >>> If XFS is going to use each of these in large quantities, then it doesn't
> >>> seem unreasonable for XFS to create a slab for each type of metadata?
> >>
> >>
> >> Well, that is the question, isn't it? How many other filesystems
> >> will want to make similar "don't use entire pages just for 4k of
> >> metadata" optimisations as 64k page size machines become more
> >> common? There are others that have the same "use slab for sector
> >> aligned IO" which will fall foul of the same problem that has been
> >> reported for XFS....
> >>
> >> If nobody else cares/wants it, then it can be XFS only. But it's
> >> only fair we address the "will it be useful to others" question
> >> first.....
> >
> > This kind of slab cache should have been global, just like interface of
> > kmalloc(size).
> >
> > However, the alignment requirement depends on block device's block size,
> > then it becomes hard to implement as genera interface, for example:
> >
> >       block size: 512, 1024, 2048, 4096
> >       slab size: 512*N, 0 < N < PAGE_SIZE/512
> >
> > For 4k page size, 28(7*4) slabs need to be created, and 64k page size
> > needs to create 127*4 slabs.
> >
>
> Where does the '*4' multiplier come from?

The buffer needs to be device block size aligned for dio, and now the block
size can be 512, 1024, 2048 and 4096.

Thanks,
Ming Lei

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Matthew Wilcox]

On Tue, Feb 26, 2019 at 07:12:49PM +0800, Ming Lei wrote:
> On Tue, Feb 26, 2019 at 6:07 PM Vlastimil Babka <vbabka@suse.cz> wrote:
> > On 2/26/19 10:33 AM, Ming Lei wrote:
> > > On Tue, Feb 26, 2019 at 03:58:26PM +1100, Dave Chinner wrote:
> > >> On Mon, Feb 25, 2019 at 07:27:37PM -0800, Matthew Wilcox wrote:
> > >>> On Tue, Feb 26, 2019 at 02:02:14PM +1100, Dave Chinner wrote:
> > >>>>> Or what is the exact size of sub-page IO in xfs most of time? For
> > >>>>
> > >>>> Determined by mkfs parameters. Any power of 2 between 512 bytes and
> > >>>> 64kB needs to be supported. e.g:
> > >>>>
> > >>>> # mkfs.xfs -s size=512 -b size=1k -i size=2k -n size=8k ....
> > >>>>
> > >>>> will have metadata that is sector sized (512 bytes), filesystem
> > >>>> block sized (1k), directory block sized (8k) and inode cluster sized
> > >>>> (32k), and will use all of them in large quantities.
> > >>>
> > >>> If XFS is going to use each of these in large quantities, then it doesn't
> > >>> seem unreasonable for XFS to create a slab for each type of metadata?
> > >>
> > >>
> > >> Well, that is the question, isn't it? How many other filesystems
> > >> will want to make similar "don't use entire pages just for 4k of
> > >> metadata" optimisations as 64k page size machines become more
> > >> common? There are others that have the same "use slab for sector
> > >> aligned IO" which will fall foul of the same problem that has been
> > >> reported for XFS....
> > >>
> > >> If nobody else cares/wants it, then it can be XFS only. But it's
> > >> only fair we address the "will it be useful to others" question
> > >> first.....
> > >
> > > This kind of slab cache should have been global, just like interface of
> > > kmalloc(size).
> > >
> > > However, the alignment requirement depends on block device's block size,
> > > then it becomes hard to implement as genera interface, for example:
> > >
> > >       block size: 512, 1024, 2048, 4096
> > >       slab size: 512*N, 0 < N < PAGE_SIZE/512
> > >
> > > For 4k page size, 28(7*4) slabs need to be created, and 64k page size
> > > needs to create 127*4 slabs.
> > >
> >
> > Where does the '*4' multiplier come from?
> 
> The buffer needs to be device block size aligned for dio, and now the block
> size can be 512, 1024, 2048 and 4096.

Why does the block size make a difference?  This requirement is due to
some storage devices having shoddy DMA controllers.  Are you saying there
are devices which can't even do 512-byte aligned I/O?

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Ming Lei]

On Tue, Feb 26, 2019 at 04:12:09AM -0800, Matthew Wilcox wrote:
> On Tue, Feb 26, 2019 at 07:12:49PM +0800, Ming Lei wrote:
> > On Tue, Feb 26, 2019 at 6:07 PM Vlastimil Babka <vbabka@suse.cz> wrote:
> > > On 2/26/19 10:33 AM, Ming Lei wrote:
> > > > On Tue, Feb 26, 2019 at 03:58:26PM +1100, Dave Chinner wrote:
> > > >> On Mon, Feb 25, 2019 at 07:27:37PM -0800, Matthew Wilcox wrote:
> > > >>> On Tue, Feb 26, 2019 at 02:02:14PM +1100, Dave Chinner wrote:
> > > >>>>> Or what is the exact size of sub-page IO in xfs most of time? For
> > > >>>>
> > > >>>> Determined by mkfs parameters. Any power of 2 between 512 bytes and
> > > >>>> 64kB needs to be supported. e.g:
> > > >>>>
> > > >>>> # mkfs.xfs -s size=512 -b size=1k -i size=2k -n size=8k ....
> > > >>>>
> > > >>>> will have metadata that is sector sized (512 bytes), filesystem
> > > >>>> block sized (1k), directory block sized (8k) and inode cluster sized
> > > >>>> (32k), and will use all of them in large quantities.
> > > >>>
> > > >>> If XFS is going to use each of these in large quantities, then it doesn't
> > > >>> seem unreasonable for XFS to create a slab for each type of metadata?
> > > >>
> > > >>
> > > >> Well, that is the question, isn't it? How many other filesystems
> > > >> will want to make similar "don't use entire pages just for 4k of
> > > >> metadata" optimisations as 64k page size machines become more
> > > >> common? There are others that have the same "use slab for sector
> > > >> aligned IO" which will fall foul of the same problem that has been
> > > >> reported for XFS....
> > > >>
> > > >> If nobody else cares/wants it, then it can be XFS only. But it's
> > > >> only fair we address the "will it be useful to others" question
> > > >> first.....
> > > >
> > > > This kind of slab cache should have been global, just like interface of
> > > > kmalloc(size).
> > > >
> > > > However, the alignment requirement depends on block device's block size,
> > > > then it becomes hard to implement as genera interface, for example:
> > > >
> > > >       block size: 512, 1024, 2048, 4096
> > > >       slab size: 512*N, 0 < N < PAGE_SIZE/512
> > > >
> > > > For 4k page size, 28(7*4) slabs need to be created, and 64k page size
> > > > needs to create 127*4 slabs.
> > > >
> > >
> > > Where does the '*4' multiplier come from?
> > 
> > The buffer needs to be device block size aligned for dio, and now the block
> > size can be 512, 1024, 2048 and 4096.
> 
> Why does the block size make a difference?  This requirement is due to
> some storage devices having shoddy DMA controllers.  Are you saying there
> are devices which can't even do 512-byte aligned I/O?

Direct IO requires that, see do_blockdev_direct_IO().

This issue can be triggered when running xfs over loop/dio. We could
fallback to buffered IO under this situation, but not sure it is the
only case.


Thanks,
Ming

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Matthew Wilcox]

On Tue, Feb 26, 2019 at 08:35:46PM +0800, Ming Lei wrote:
> On Tue, Feb 26, 2019 at 04:12:09AM -0800, Matthew Wilcox wrote:
> > On Tue, Feb 26, 2019 at 07:12:49PM +0800, Ming Lei wrote:
> > > The buffer needs to be device block size aligned for dio, and now the block
> > > size can be 512, 1024, 2048 and 4096.
> > 
> > Why does the block size make a difference?  This requirement is due to
> > some storage devices having shoddy DMA controllers.  Are you saying there
> > are devices which can't even do 512-byte aligned I/O?
> 
> Direct IO requires that, see do_blockdev_direct_IO().
> 
> This issue can be triggered when running xfs over loop/dio. We could
> fallback to buffered IO under this situation, but not sure it is the
> only case.

Wait, we're imposing a ridiculous amount of complexity on XFS for no
reason at all?  We should just change this to 512-byte alignment.  Tying
it to the blocksize of the device never made any sense.

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Ming Lei]

On Tue, Feb 26, 2019 at 05:02:30AM -0800, Matthew Wilcox wrote:
> On Tue, Feb 26, 2019 at 08:35:46PM +0800, Ming Lei wrote:
> > On Tue, Feb 26, 2019 at 04:12:09AM -0800, Matthew Wilcox wrote:
> > > On Tue, Feb 26, 2019 at 07:12:49PM +0800, Ming Lei wrote:
> > > > The buffer needs to be device block size aligned for dio, and now the block
> > > > size can be 512, 1024, 2048 and 4096.
> > > 
> > > Why does the block size make a difference?  This requirement is due to
> > > some storage devices having shoddy DMA controllers.  Are you saying there
> > > are devices which can't even do 512-byte aligned I/O?
> > 
> > Direct IO requires that, see do_blockdev_direct_IO().
> > 
> > This issue can be triggered when running xfs over loop/dio. We could
> > fallback to buffered IO under this situation, but not sure it is the
> > only case.
> 
> Wait, we're imposing a ridiculous amount of complexity on XFS for no
> reason at all?  We should just change this to 512-byte alignment.  Tying
> it to the blocksize of the device never made any sense.

OK, that is fine since we can fallback to buffered IO for loop in case of
unaligned dio.

Then something like the following patch should work for all fs, could
anyone comment on this approach?

--

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 5f2c429d4378..76f09f23a410 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -405,3 +405,44 @@ int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
 	return ret;
 }
 EXPORT_SYMBOL(blkdev_issue_zeroout);
+
+static struct kmem_cache *sector_buf_slabs[(PAGE_SIZE >> 9) - 1];
+
+void *blk_alloc_sec_buf(unsigned size, gfp_t flags)
+{
+	int idx;
+
+	size = round_up(size, 512);
+	if (size >= PAGE_SIZE)
+		return NULL;
+
+	idx = (size >> 9) - 1;
+	if (!sector_buf_slabs[idx])
+		return NULL;
+	return kmem_cache_alloc(sector_buf_slabs[idx], flags);
+}
+EXPORT_SYMBOL_GPL(blk_alloc_sec_buf);
+
+void blk_free_sec_buf(void *buf, int size)
+{
+	size = round_up(size, 512);
+	if (size >= PAGE_SIZE)
+		return;
+
+	return kmem_cache_free(sector_buf_slabs[(size >> 9) - 1], buf);
+}
+EXPORT_SYMBOL_GPL(blk_free_sec_buf);
+
+void __init blk_sector_buf_init(void)
+{
+	unsigned size;
+
+	for (size = 512; size < PAGE_SIZE; size += 512) {
+		char name[16];
+		int idx = (size >> 9) - 1;
+
+		snprintf(name, 16, "blk_sec_buf-%u", size);
+		sector_buf_slabs[idx] = kmem_cache_create(name, size, 512,
+							  SLAB_PANIC, NULL);
+	}
+}
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index faed9d9eb84c..a4117e526715 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1657,6 +1657,9 @@ extern int bdev_read_page(struct block_device *, sector_t, struct page *);
 extern int bdev_write_page(struct block_device *, sector_t, struct page *,
 						struct writeback_control *);
 
+extern void *blk_alloc_sec_buf(unsigned size, gfp_t flags);
+extern void blk_free_sec_buf(void *buf, int size);
+
 #ifdef CONFIG_BLK_DEV_ZONED
 bool blk_req_needs_zone_write_lock(struct request *rq);
 void __blk_req_zone_write_lock(struct request *rq);
@@ -1755,6 +1758,15 @@ static inline int blkdev_issue_flush(struct block_device *bdev, gfp_t gfp_mask,
 	return 0;
 }
 
+static inline void *blk_alloc_sec_buf(unsigned size, gfp_t flags)
+{
+	return NULL;
+}
+
+static inline void blk_free_sec_buf(void *buf, int size)
+{
+}
+
 #endif /* CONFIG_BLOCK */
 
 static inline void blk_wake_io_task(struct task_struct *waiter)

Thanks,
Ming

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Matthew Wilcox]

On Tue, Feb 26, 2019 at 09:42:48PM +0800, Ming Lei wrote:
> On Tue, Feb 26, 2019 at 05:02:30AM -0800, Matthew Wilcox wrote:
> > Wait, we're imposing a ridiculous amount of complexity on XFS for no
> > reason at all?  We should just change this to 512-byte alignment.  Tying
> > it to the blocksize of the device never made any sense.
> 
> OK, that is fine since we can fallback to buffered IO for loop in case of
> unaligned dio.
> 
> Then something like the following patch should work for all fs, could
> anyone comment on this approach?

That's not even close to what I meant.

diff --git a/fs/direct-io.c b/fs/direct-io.c
index ec2fb6fe6d37..dee1fc47a7fc 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -1185,18 +1185,20 @@ do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
 	struct dio_submit sdio = { 0, };
 	struct buffer_head map_bh = { 0, };
 	struct blk_plug plug;
-	unsigned long align = offset | iov_iter_alignment(iter);
 
 	/*
 	 * Avoid references to bdev if not absolutely needed to give
 	 * the early prefetch in the caller enough time.
 	 */
 
-	if (align & blocksize_mask) {
+	if (iov_iter_alignment(iter) & 511)
+		goto out;
+
+	if (offset & blocksize_mask) {
 		if (bdev)
 			blkbits = blksize_bits(bdev_logical_block_size(bdev));
 		blocksize_mask = (1 << blkbits) - 1;
-		if (align & blocksize_mask)
+		if (offset & blocksize_mask)
 			goto out;
 	}
 

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Darrick J. Wong]

On Tue, Feb 26, 2019 at 06:04:40AM -0800, Matthew Wilcox wrote:
> On Tue, Feb 26, 2019 at 09:42:48PM +0800, Ming Lei wrote:
> > On Tue, Feb 26, 2019 at 05:02:30AM -0800, Matthew Wilcox wrote:
> > > Wait, we're imposing a ridiculous amount of complexity on XFS for no
> > > reason at all?  We should just change this to 512-byte alignment.  Tying
> > > it to the blocksize of the device never made any sense.
> > 
> > OK, that is fine since we can fallback to buffered IO for loop in case of
> > unaligned dio.
> > 
> > Then something like the following patch should work for all fs, could
> > anyone comment on this approach?
> 
> That's not even close to what I meant.
> 
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index ec2fb6fe6d37..dee1fc47a7fc 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -1185,18 +1185,20 @@ do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,

Wait a minute, are you all saying that /directio/ is broken on XFS too??
XFS doesn't use blockdev_direct_IO anymore.

I thought we were talking about alignment of XFS metadata buffers
(xfs_buf.c), which is a very different topic.

As I understand the problem, in non-debug mode the slab caches give
xfs_buf chunks of memory that are aligned well enough to work, but in
debug mode the slabs allocate slightly more bytes to carry debug
information which pushes the returned address up slightly, thus breaking
the alignment requirements.

So why can't we just move the debug info to the end of the object?  If
our 512 byte allocation turns into a (512 + a few more) bytes we'll end
up using 1024 bytes on the allocation regardless, so it shouldn't matter
to put the debug info at offset 512.  If the reason is fear that kernel
code will scribble off the end of the object, then return (*obj + 512).
Maybe you all have already covered this, though?

--D

>  	struct dio_submit sdio = { 0, };
>  	struct buffer_head map_bh = { 0, };
>  	struct blk_plug plug;
> -	unsigned long align = offset | iov_iter_alignment(iter);
>  
>  	/*
>  	 * Avoid references to bdev if not absolutely needed to give
>  	 * the early prefetch in the caller enough time.
>  	 */
>  
> -	if (align & blocksize_mask) {
> +	if (iov_iter_alignment(iter) & 511)
> +		goto out;
> +
> +	if (offset & blocksize_mask) {
>  		if (bdev)
>  			blkbits = blksize_bits(bdev_logical_block_size(bdev));
>  		blocksize_mask = (1 << blkbits) - 1;
> -		if (align & blocksize_mask)
> +		if (offset & blocksize_mask)
>  			goto out;
>  	}
>  

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Matthew Wilcox]

On Tue, Feb 26, 2019 at 08:14:33AM -0800, Darrick J. Wong wrote:
> On Tue, Feb 26, 2019 at 06:04:40AM -0800, Matthew Wilcox wrote:
> > On Tue, Feb 26, 2019 at 09:42:48PM +0800, Ming Lei wrote:
> > > On Tue, Feb 26, 2019 at 05:02:30AM -0800, Matthew Wilcox wrote:
> > > > Wait, we're imposing a ridiculous amount of complexity on XFS for no
> > > > reason at all?  We should just change this to 512-byte alignment.  Tying
> > > > it to the blocksize of the device never made any sense.
> > > 
> > > OK, that is fine since we can fallback to buffered IO for loop in case of
> > > unaligned dio.
> > > 
> > > Then something like the following patch should work for all fs, could
> > > anyone comment on this approach?
> > 
> > That's not even close to what I meant.
> > 
> > diff --git a/fs/direct-io.c b/fs/direct-io.c
> > index ec2fb6fe6d37..dee1fc47a7fc 100644
> > --- a/fs/direct-io.c
> > +++ b/fs/direct-io.c
> > @@ -1185,18 +1185,20 @@ do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
> 
> Wait a minute, are you all saying that /directio/ is broken on XFS too??
> XFS doesn't use blockdev_direct_IO anymore.
> 
> I thought we were talking about alignment of XFS metadata buffers
> (xfs_buf.c), which is a very different topic.
> 
> As I understand the problem, in non-debug mode the slab caches give
> xfs_buf chunks of memory that are aligned well enough to work, but in
> debug mode the slabs allocate slightly more bytes to carry debug
> information which pushes the returned address up slightly, thus breaking
> the alignment requirements.
> 
> So why can't we just move the debug info to the end of the object?  If
> our 512 byte allocation turns into a (512 + a few more) bytes we'll end
> up using 1024 bytes on the allocation regardless, so it shouldn't matter
> to put the debug info at offset 512.  If the reason is fear that kernel
> code will scribble off the end of the object, then return (*obj + 512).
> Maybe you all have already covered this, though?

I don't know _what_ Ming Lei is saying.  I thought the problem was
with slab redzones, which need to be before and after each object,
but apparently the problem is with KASAN as well.

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Ming Lei]

On Tue, Feb 26, 2019 at 08:19:12AM -0800, Matthew Wilcox wrote:
> On Tue, Feb 26, 2019 at 08:14:33AM -0800, Darrick J. Wong wrote:
> > On Tue, Feb 26, 2019 at 06:04:40AM -0800, Matthew Wilcox wrote:
> > > On Tue, Feb 26, 2019 at 09:42:48PM +0800, Ming Lei wrote:
> > > > On Tue, Feb 26, 2019 at 05:02:30AM -0800, Matthew Wilcox wrote:
> > > > > Wait, we're imposing a ridiculous amount of complexity on XFS for no
> > > > > reason at all?  We should just change this to 512-byte alignment.  Tying
> > > > > it to the blocksize of the device never made any sense.
> > > > 
> > > > OK, that is fine since we can fallback to buffered IO for loop in case of
> > > > unaligned dio.
> > > > 
> > > > Then something like the following patch should work for all fs, could
> > > > anyone comment on this approach?
> > > 
> > > That's not even close to what I meant.
> > > 
> > > diff --git a/fs/direct-io.c b/fs/direct-io.c
> > > index ec2fb6fe6d37..dee1fc47a7fc 100644
> > > --- a/fs/direct-io.c
> > > +++ b/fs/direct-io.c
> > > @@ -1185,18 +1185,20 @@ do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
> > 
> > Wait a minute, are you all saying that /directio/ is broken on XFS too??
> > XFS doesn't use blockdev_direct_IO anymore.
> > 
> > I thought we were talking about alignment of XFS metadata buffers
> > (xfs_buf.c), which is a very different topic.
> > 
> > As I understand the problem, in non-debug mode the slab caches give
> > xfs_buf chunks of memory that are aligned well enough to work, but in
> > debug mode the slabs allocate slightly more bytes to carry debug
> > information which pushes the returned address up slightly, thus breaking
> > the alignment requirements.
> > 
> > So why can't we just move the debug info to the end of the object?  If
> > our 512 byte allocation turns into a (512 + a few more) bytes we'll end
> > up using 1024 bytes on the allocation regardless, so it shouldn't matter
> > to put the debug info at offset 512.  If the reason is fear that kernel
> > code will scribble off the end of the object, then return (*obj + 512).
> > Maybe you all have already covered this, though?
> 
> I don't know _what_ Ming Lei is saying.  I thought the problem was
> with slab redzones, which need to be before and after each object,
> but apparently the problem is with KASAN as well.

I have mentioned several times that it is triggered on xfs over
loop/dio, however it may be addressed by falling back to buffered IO
in case unaligned buffer.

Please see lo_rw_aio().

Thanks,
Ming

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Vlastimil Babka]

On 2/26/19 5:19 PM, Matthew Wilcox wrote:
> On Tue, Feb 26, 2019 at 08:14:33AM -0800, Darrick J. Wong wrote:
>> On Tue, Feb 26, 2019 at 06:04:40AM -0800, Matthew Wilcox wrote:
>> Wait a minute, are you all saying that /directio/ is broken on XFS too??
>> XFS doesn't use blockdev_direct_IO anymore.
>>
>> I thought we were talking about alignment of XFS metadata buffers
>> (xfs_buf.c), which is a very different topic.
>>
>> As I understand the problem, in non-debug mode the slab caches give
>> xfs_buf chunks of memory that are aligned well enough to work, but in
>> debug mode the slabs allocate slightly more bytes to carry debug
>> information which pushes the returned address up slightly, thus breaking
>> the alignment requirements.
>>
>> So why can't we just move the debug info to the end of the object?  If
>> our 512 byte allocation turns into a (512 + a few more) bytes we'll end
>> up using 1024 bytes on the allocation regardless, so it shouldn't matter
>> to put the debug info at offset 512.  If the reason is fear that kernel
>> code will scribble off the end of the object, then return (*obj + 512).
>> Maybe you all have already covered this, though?
> 
> I don't know _what_ Ming Lei is saying.  I thought the problem was
> with slab redzones, which need to be before and after each object,
> but apparently the problem is with KASAN as well.

That's what I thought as well. But if we can solve it for caches created
by kmem_cache_create(..., align, ...) then IMHO we could guarantee
natural alignment for power-of-two kmalloc caches as well.

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Christoph Hellwig]

On Wed, Feb 27, 2019 at 08:07:31AM +0100, Vlastimil Babka wrote:
> > I don't know _what_ Ming Lei is saying.  I thought the problem was
> > with slab redzones, which need to be before and after each object,
> > but apparently the problem is with KASAN as well.
> 
> That's what I thought as well. But if we can solve it for caches created
> by kmem_cache_create(..., align, ...) then IMHO we could guarantee
> natural alignment for power-of-two kmalloc caches as well.

Yes, having a version of kmalloc that guarantees power of two alignment
would be extremely helpful and avoid a lot of pointless boilerplate code.

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Dave Chinner]

On Tue, Feb 26, 2019 at 08:14:33AM -0800, Darrick J. Wong wrote:
> On Tue, Feb 26, 2019 at 06:04:40AM -0800, Matthew Wilcox wrote:
> > On Tue, Feb 26, 2019 at 09:42:48PM +0800, Ming Lei wrote:
> > > On Tue, Feb 26, 2019 at 05:02:30AM -0800, Matthew Wilcox wrote:
> > > > Wait, we're imposing a ridiculous amount of complexity on XFS for no
> > > > reason at all?  We should just change this to 512-byte alignment.  Tying
> > > > it to the blocksize of the device never made any sense.
> > > 
> > > OK, that is fine since we can fallback to buffered IO for loop in case of
> > > unaligned dio.
> > > 
> > > Then something like the following patch should work for all fs, could
> > > anyone comment on this approach?
> > 
> > That's not even close to what I meant.
> > 
> > diff --git a/fs/direct-io.c b/fs/direct-io.c
> > index ec2fb6fe6d37..dee1fc47a7fc 100644
> > --- a/fs/direct-io.c
> > +++ b/fs/direct-io.c
> > @@ -1185,18 +1185,20 @@ do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
> 
> Wait a minute, are you all saying that /directio/ is broken on XFS too??
> XFS doesn't use blockdev_direct_IO anymore.

No, loop devices w/ direct IO is a complete red herring. It's the
same issue - the upper filesystem is sending down unaligned bios
to the loop device, which is then just passing them on to the
underlying filesystem via DIO, and the DIO code in the lower
filesystem saying "that user memory buffer ain't aligned to my
logical sector size" and rejecting it.

Actually, in XFS's case, it doesn't care about memory buffer
alignment - it's the iomap code that is rejecting it when mapping
the memory buffer to a bio in iomap_dio_bio_actor():

	unsigned int blkbits = blksize_bits(bdev_logical_block_size(iomap->bdev));
.....
	unsigned int align = iov_iter_alignment(dio->submit.iter);
.....
	if ((pos | length | align) & ((1 << blkbits) - 1))
		return -EINVAL;

IOWs, if the memory buffer is not aligned to the logical block size
of the underlying device (which defaults to 512 bytes) then it will
be rejected by the lower filesystem...

> I thought we were talking about alignment of XFS metadata buffers
> (xfs_buf.c), which is a very different topic.

Yup, it's the same problem, just a different symptom. Fix the
unaligned buffer problem, we fix the loop dev w/ direct-io problem,
too.

FWIW, this "filesystem image sector size mismatch" problem has been
around for many, many years - the same issue that occurs with loop
devices when you try to mount a 512 byte sector image on a hard 4k
sector host filesystem/storage device using direct IO in the loop
device. This isn't a new thing at all - if you want to use direct IO
to manipulate filesystem images, you actually need to know what you
are doing....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Christopher Lameter]

On Tue, 26 Feb 2019, Ming Lei wrote:

> Then something like the following patch should work for all fs, could
> anyone comment on this approach?

Note that various subsystems have similar implementations. Have a look at

drivers/dma/dmaengine.c

struct dmaengine_unmap_pool {
        struct kmem_cache *cache;
        const char *name;
        mempool_t *pool;
        size_t size;
};

#define __UNMAP_POOL(x) { .size = x, .name = "dmaengine-unmap-"
__stringify(x) }
static struct dmaengine_unmap_pool unmap_pool[] = {
        __UNMAP_POOL(2),
        #if IS_ENABLED(CONFIG_DMA_ENGINE_RAID)
        __UNMAP_POOL(16),
        __UNMAP_POOL(128),
        __UNMAP_POOL(256),
        #endif
};

Or drivers/md/dm-bufio.c:

struct dm_bufio_client {
        struct mutex lock;

        struct list_head lru[LIST_SIZE];
        unsigned long n_buffers[LIST_SIZE];

        struct block_device *bdev;
        unsigned block_size;
        s8 sectors_per_block_bits;
        void (*alloc_callback)(struct dm_buffer *);
        void (*write_callback)(struct dm_buffer *);

        struct kmem_cache *slab_buffer;
        struct kmem_cache *slab_cache;
        struct dm_io_client *dm_io;

        struct list_head reserved_buffers;
        unsigned need_reserved_buffers;

        unsigned minimum_buffers;

        struct rb_root buffer_tree;
        wait_queue_head_t free_buffer_wait;

        sector_t start;

        int async_write_error;

        struct list_head client_list;
        struct shrinker shrinker;
};

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Dave Chinner]

On Tue, Feb 26, 2019 at 05:33:04PM +0800, Ming Lei wrote:
> On Tue, Feb 26, 2019 at 03:58:26PM +1100, Dave Chinner wrote:
> > On Mon, Feb 25, 2019 at 07:27:37PM -0800, Matthew Wilcox wrote:
> > > On Tue, Feb 26, 2019 at 02:02:14PM +1100, Dave Chinner wrote:
> > > > > Or what is the exact size of sub-page IO in xfs most of time? For
> > > > 
> > > > Determined by mkfs parameters. Any power of 2 between 512 bytes and
> > > > 64kB needs to be supported. e.g:
> > > > 
> > > > # mkfs.xfs -s size=512 -b size=1k -i size=2k -n size=8k ....
> > > > 
> > > > will have metadata that is sector sized (512 bytes), filesystem
> > > > block sized (1k), directory block sized (8k) and inode cluster sized
> > > > (32k), and will use all of them in large quantities.
> > > 
> > > If XFS is going to use each of these in large quantities, then it doesn't
> > > seem unreasonable for XFS to create a slab for each type of metadata?
> > 
> > 
> > Well, that is the question, isn't it? How many other filesystems
> > will want to make similar "don't use entire pages just for 4k of
> > metadata" optimisations as 64k page size machines become more
> > common? There are others that have the same "use slab for sector
> > aligned IO" which will fall foul of the same problem that has been
> > reported for XFS....
> > 
> > If nobody else cares/wants it, then it can be XFS only. But it's
> > only fair we address the "will it be useful to others" question
> > first.....
> 
> This kind of slab cache should have been global, just like interface of
> kmalloc(size).
> 
> However, the alignment requirement depends on block device's block size,
> then it becomes hard to implement as genera interface, for example:
> 
> 	block size: 512, 1024, 2048, 4096
> 	slab size: 512*N, 0 < N < PAGE_SIZE/512
> 
> For 4k page size, 28(7*4) slabs need to be created, and 64k page size
> needs to create 127*4 slabs.

IDGI. Where's the 7/127 come from?

We only require sector alignment at most, so as long as each slab
object is aligned to it's size, we only need one slab for each block
size.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Ming Lei]

On Wed, Feb 27, 2019 at 07:45:50AM +1100, Dave Chinner wrote:
> On Tue, Feb 26, 2019 at 05:33:04PM +0800, Ming Lei wrote:
> > On Tue, Feb 26, 2019 at 03:58:26PM +1100, Dave Chinner wrote:
> > > On Mon, Feb 25, 2019 at 07:27:37PM -0800, Matthew Wilcox wrote:
> > > > On Tue, Feb 26, 2019 at 02:02:14PM +1100, Dave Chinner wrote:
> > > > > > Or what is the exact size of sub-page IO in xfs most of time? For
> > > > > 
> > > > > Determined by mkfs parameters. Any power of 2 between 512 bytes and
> > > > > 64kB needs to be supported. e.g:
> > > > > 
> > > > > # mkfs.xfs -s size=512 -b size=1k -i size=2k -n size=8k ....
> > > > > 
> > > > > will have metadata that is sector sized (512 bytes), filesystem
> > > > > block sized (1k), directory block sized (8k) and inode cluster sized
> > > > > (32k), and will use all of them in large quantities.
> > > > 
> > > > If XFS is going to use each of these in large quantities, then it doesn't
> > > > seem unreasonable for XFS to create a slab for each type of metadata?
> > > 
> > > 
> > > Well, that is the question, isn't it? How many other filesystems
> > > will want to make similar "don't use entire pages just for 4k of
> > > metadata" optimisations as 64k page size machines become more
> > > common? There are others that have the same "use slab for sector
> > > aligned IO" which will fall foul of the same problem that has been
> > > reported for XFS....
> > > 
> > > If nobody else cares/wants it, then it can be XFS only. But it's
> > > only fair we address the "will it be useful to others" question
> > > first.....
> > 
> > This kind of slab cache should have been global, just like interface of
> > kmalloc(size).
> > 
> > However, the alignment requirement depends on block device's block size,
> > then it becomes hard to implement as genera interface, for example:
> > 
> > 	block size: 512, 1024, 2048, 4096
> > 	slab size: 512*N, 0 < N < PAGE_SIZE/512
> > 
> > For 4k page size, 28(7*4) slabs need to be created, and 64k page size
> > needs to create 127*4 slabs.
> 
> IDGI. Where's the 7/127 come from?
> 
> We only require sector alignment at most, so as long as each slab
> object is aligned to it's size, we only need one slab for each block
> size.

Each slab has fixed size, I remembered that you mentioned that the meta
data size can be 512 * N (1 <= N <= PAGE_SIZE / 512).

https://marc.info/?l=linux-fsdevel&m=155115014513355&w=2


Thanks, 
Ming

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Dave Chinner]

On Wed, Feb 27, 2019 at 09:50:55AM +0800, Ming Lei wrote:
> On Wed, Feb 27, 2019 at 07:45:50AM +1100, Dave Chinner wrote:
> > On Tue, Feb 26, 2019 at 05:33:04PM +0800, Ming Lei wrote:
> > > On Tue, Feb 26, 2019 at 03:58:26PM +1100, Dave Chinner wrote:
> > > > On Mon, Feb 25, 2019 at 07:27:37PM -0800, Matthew Wilcox wrote:
> > > > > On Tue, Feb 26, 2019 at 02:02:14PM +1100, Dave Chinner wrote:
> > > > > > > Or what is the exact size of sub-page IO in xfs most of time? For
> > > > > > 
> > > > > > Determined by mkfs parameters. Any power of 2 between 512 bytes and
> > > > > > 64kB needs to be supported. e.g:
> > > > > > 
> > > > > > # mkfs.xfs -s size=512 -b size=1k -i size=2k -n size=8k ....
> > > > > > 
> > > > > > will have metadata that is sector sized (512 bytes), filesystem
> > > > > > block sized (1k), directory block sized (8k) and inode cluster sized
> > > > > > (32k), and will use all of them in large quantities.
> > > > > 
> > > > > If XFS is going to use each of these in large quantities, then it doesn't
> > > > > seem unreasonable for XFS to create a slab for each type of metadata?
> > > > 
> > > > 
> > > > Well, that is the question, isn't it? How many other filesystems
> > > > will want to make similar "don't use entire pages just for 4k of
> > > > metadata" optimisations as 64k page size machines become more
> > > > common? There are others that have the same "use slab for sector
> > > > aligned IO" which will fall foul of the same problem that has been
> > > > reported for XFS....
> > > > 
> > > > If nobody else cares/wants it, then it can be XFS only. But it's
> > > > only fair we address the "will it be useful to others" question
> > > > first.....
> > > 
> > > This kind of slab cache should have been global, just like interface of
> > > kmalloc(size).
> > > 
> > > However, the alignment requirement depends on block device's block size,
> > > then it becomes hard to implement as genera interface, for example:
> > > 
> > > 	block size: 512, 1024, 2048, 4096
> > > 	slab size: 512*N, 0 < N < PAGE_SIZE/512
> > > 
> > > For 4k page size, 28(7*4) slabs need to be created, and 64k page size
> > > needs to create 127*4 slabs.
> > 
> > IDGI. Where's the 7/127 come from?
> > 
> > We only require sector alignment at most, so as long as each slab
> > object is aligned to it's size, we only need one slab for each block
> > size.
> 
> Each slab has fixed size, I remembered that you mentioned that the meta
> data size can be 512 * N (1 <= N <= PAGE_SIZE / 512).
> 
> https://marc.info/?l=linux-fsdevel&m=155115014513355&w=2

nggggh. 

*That* *is* *not* *what* *I* *said*.

That is *what you said* and I said that was wrong and the actual
sizes needed were:

dgc> It is not. On a 64k page size machine, we use sub page slabs for
dgc> metadata blocks of 2^N bytes where 9 <= N <= 15..

Please do the maths. I shoul dnot have to do it for you.

Also, please don't confusing sector-in-LBA alignment with /memory
buffer/ alignment. i.e. you're assuming that these 2kB IOs at
different sector offsets like the following:

 0	0.5	1.0	1.5	2.0	2.5	3.0	3.5	4.0
 +-------+-------+-------+-------+-------+-------+-------+-------+
 +ooooooooooooooooooooooooooooooo+
	 +ooooooooooooooooooooooooooooooo+
		 +ooooooooooooooooooooooooooooooo+
			 +ooooooooooooooooooooooooooooooo+
				 +ooooooooooooooooooooooooooooooo+

require a memory buffer alignment that matches the sector-in-LBA
alignment. This is wrong - the memory alignment constraint is
usually a hardware DMA engine limitation and has nothing to do with
the LBA the IO will place/retreive the data in/from.

i.e. An aligned 2kB slab will only allocate 2kB slab objects like so:

 0	0.5	1.0	1.5	2.0	2.5	3.0	3.5	4.0
 +-------+-------+-------+-------+-------+-------+-------+-------+
 +ooooooooooooooooooooooooooooooo+
				 +ooooooooooooooooooooooooooooooo+

and these memory buffers will always have 512 byte alignment. This
meets the memory alignemnt requirements of all hardware (which is
512 byte alignment or smaller), and so we only need one slab per
size.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: allocate sector sized IO buffer via page_frag_alloc

[Christopher Lameter]

On Mon, 25 Feb 2019, Vlastimil Babka wrote:

> What about kmem_cache_create() with align parameter? That *should* be
> guaranteed regardless of whatever debugging is enabled - if not, I would
> consider it a bug.

It definitely guarantees that. What would be the point of the alignment
parameter otherwise?