xfs: use aligned buffers for IO
PATCH v2 1/1 xfs: use aligned buffers for IO
Hi folks,
This is an updated version of the patchset originally posted here:
https://lore.kernel.org/linux-xfs/[email protected]/T/#t
It intends to avoid th eproblems of IO being silently corrupted by drivers when kernel memory debugging options are turned on due to changes in heap allocated object alignment. Full description is in the link above.
Changes in V2
- dropped xfs_add_bio_page() validation wrapper. Contentious, Christoph will look to adding it into the generic block layer code.
- added xfs_buftarg_dma_alignment() to grab the alignment from the current device we are allocating a buffer for.
- feed the correct alignment for the underlying device into kmem_alloc_io() to minimise the occurrence of failed alignment for devices that support fine-grained alignment.
- kmem_alloc_io() supports alignment up to page size and will warn if any alignment greater than a page is requested. Devices that require larger than page alignment should not exist.
Cheers,
Dave.
PATCH 1/3 xfs: add kmem allocation trace points
From: Dave Chinner <[email protected]>
When trying to correlate XFS kernel allocations to memory reclaim behaviour, it is useful to know what allocations XFS is actually attempting. This information is not directly available from tracepoints in the generic memory allocation and reclaim tracepoints, so these new trace points provide a high level indication of what the XFS memory demand actually is.
There is no per-filesystem context in this code, so we just trace the type of allocation, the size and the allocation constraints. The kmem code also doesn't include much of the common XFS headers, so there are a few definitions that need to be added to the trace headers and a couple of types that need to be made common to avoid needing to include the whole world in the kmem code.
--- fs/xfs/kmem.c | 11 +++++++++-- fs/xfs/libxfs/xfs_types.h | 8 ++++++++ fs/xfs/xfs_mount.h | 7 ------- fs/xfs/xfs_trace.h | 33 +++++++++++++++++++++++++++++++++ 4 files changed, 50 insertions(+), 9 deletions(-)
fs/xfs/kmem.c +9 -2
@@ -3,10 +3,10 @@* Copyright (c) 2000-2005 Silicon Graphics, Inc. * All Rights Reserved. */ -#include <linux/sched/mm.h> +#include "xfs.h" #include <linux/backing-dev.h> -#include "kmem.h" #include "xfs_message.h" +#include "xfs_trace.h" void *kmem_alloc (size_t size,xfs_km_flags_t flags)
@@ -15,6 +15,8 @@ kmem_alloc(size_t size, xfs_km_flags_t flags)gfp_t lflags =kmem_flags_convert (flags);void *ptr; +trace_kmem_alloc (size, flags, _RET_IP_); + do { ptr = kmalloc(size, lflags); if (ptr || (flags & (KM_MAYFAIL|KM_NOSLEEP)))
@@ -35,6 +37,8 @@ kmem_alloc_large(size_t size, xfs_km_flags_t flags)void *ptr;gfp_t lflags; +trace_kmem_alloc_large (size, flags, _RET_IP_); + ptr =kmem_alloc (size, flags |KM_MAYFAIL );if (ptr)return ptr;
@@ -65,6 +69,8 @@ kmem_realloc(const void *old, size_t newsize, xfs_km_flags_t flags)gfp_t lflags =kmem_flags_convert (flags);void *ptr; +trace_kmem_realloc (newsize, flags, _RET_IP_); + do { ptr = krealloc(old, newsize, lflags); if (ptr || (flags & (KM_MAYFAIL|KM_NOSLEEP)))
@@ -85,6 +91,7 @@ kmem_zone_alloc(kmem_zone_t *zone, xfs_km_flags_t flags)gfp_t lflags =kmem_flags_convert (flags);void *ptr; +trace_kmem_zone_alloc (kmem_cache_size (zone), flags, _RET_IP_); do { ptr = kmem_cache_alloc(zone, lflags); if (ptr || (flags & (KM_MAYFAIL|KM_NOSLEEP)))
fs/xfs/libxfs/xfs_types.h +8 -0
@@ -169,6 +169,14 @@ typedef struct xfs_bmbt_irecxfs_exntst_t br_state;/* extent state */ } xfs_bmbt_irec_t; +/* per-AG block reservation types */ +enum xfs_ag_resv_type { +XFS_AG_RESV_NONE =0 , +XFS_AG_RESV_AGFL , +XFS_AG_RESV_METADATA , +XFS_AG_RESV_RMAPBT , +}; +/* * Type verifier functions */
fs/xfs/xfs_mount.h +0 -7
@@ -327,13 +327,6 @@ xfs_daddr_to_agbno(struct xfs_mount *mp, xfs_daddr_t d) }/* per-AG block reservation data structures*/ -enum xfs_ag_resv_type { -XFS_AG_RESV_NONE =0 , -XFS_AG_RESV_AGFL , -XFS_AG_RESV_METADATA , -XFS_AG_RESV_RMAPBT , -}; -struct xfs_ag_resv {/* number of blocks originally reserved here */ xfs_extlen_t ar_orig_reserved;
fs/xfs/xfs_trace.h +33 -0
@@ -23,6 +23,7 @@ struct xlog;struct xlog_ticket ;struct xlog_recover ;struct xlog_recover_item ; +struct xlog_rec_header ;struct xfs_buf_log_format ;struct xfs_inode_log_format ;struct xfs_bmbt_irec ;
@@ -30,6 +31,10 @@ struct xfs_btree_cur;struct xfs_refcount_irec ;struct xfs_fsmap ;struct xfs_rmap_irec ; +struct xfs_icreate_log ; +struct xfs_owner_info ; +struct xfs_trans_res ; +struct xfs_inobt_rec_incore ; DECLARE_EVENT_CLASS(xfs_attr_list_class, TP_PROTO(struct xfs_attr_list_context *ctx),
@@ -3575,6 +3580,34 @@ TRACE_EVENT(xfs_pwork_init, __entry->nr_threads, __entry->pid) ) +DECLARE_EVENT_CLASS (xfs_kmem_class, + TP_PROTO(ssize_t size, int flags, unsigned long caller_ip), +TP_ARGS (size, flags, caller_ip), +TP_STRUCT__entry ( + __field(ssize_t, size) + __field(int, flags) + __field(unsigned long, caller_ip) + ), +TP_fast_assign ( + __entry->size = size; + __entry->flags = flags; + __entry->caller_ip = caller_ip; + ), +TP_printk ("size %zd flags 0x%x caller %pS" , + __entry->size, + __entry->flags, + (char *)__entry->caller_ip) +) + +#define DEFINE_KMEM_EVENT (name) \ +DEFINE_EVENT(xfs_kmem_class, name, \ + TP_PROTO(ssize_t size, int flags, unsigned long caller_ip), \ + TP_ARGS(size, flags, caller_ip)) +DEFINE_KMEM_EVENT (kmem_alloc); +DEFINE_KMEM_EVENT (kmem_alloc_large); +DEFINE_KMEM_EVENT (kmem_realloc); +DEFINE_KMEM_EVENT (kmem_zone_alloc); +#endif /* _TRACE_XFS_H */ #undef TRACE_INCLUDE_PATH
Signature · 1 line
2.23.0.rc1
Re: [PATCH 1/3] xfs: add kmem allocation trace points
Quoted 16 lines · Dave Chinner
From: Dave Chinner <[email protected]>
When trying to correlate XFS kernel allocations to memory reclaim behaviour, it is useful to know what allocations XFS is actually attempting. This information is not directly available from tracepoints in the generic memory allocation and reclaim tracepoints, so these new trace points provide a high level indication of what the XFS memory demand actually is.
There is no per-filesystem context in this code, so we just trace the type of allocation, the size and the allocation constraints. The kmem code also doesn't include much of the common XFS headers, so there are a few definitions that need to be added to the trace headers and a couple of types that need to be made common to avoid needing to include the whole world in the kmem code.
Quoted 2 lines
Signed-off-by Dave Chinner <[email protected]>Reviewed-by Brian Foster <[email protected]>
Looks good, although I'd still prefer the AG enum move in a separate prep patch:
PATCH 2/3 xfs: get allocation alignment from the buftarg
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
Needed to feed into the allocation routine to guarantee the memory buffers we add to bios are correctly aligned to the underlying device.
--- fs/xfs/xfs_buf.h | 6 ++++++ 1 file changed, 6 insertions(+)
fs/xfs/xfs_buf.h +6 -0
@@ -350,6 +350,12 @@ extern int xfs_setsize_buftarg(xfs_buftarg_t *, unsigned int);#define xfs_getsize_buftarg (buftarg) block_size((buftarg)->bt_bdev)#define xfs_readonly_buftarg (buftarg) bdev_read_only((buftarg)->bt_bdev) +static inline int +xfs_buftarg_dma_alignment (struct xfs_buftarg *bt) +{ +return queue_dma_alignment (bt->bt_bdev->bd_disk->queue); +} +int xfs_buf_reverify (struct xfs_buf *bp,const struct xfs_buf_ops *ops);bool xfs_verify_magic (struct xfs_buf *bp,__be32 dmagic);bool xfs_verify_magic16 (struct xfs_buf *bp,__be16 dmagic);
Signature · 1 line
2.23.0.rc1
Re: [PATCH 2/3] xfs: get allocation alignment from the buftarg
Quoted 7 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Needed to feed into the allocation routine to guarantee the memory buffers we add to bios are correctly aligned to the underlying device.
Signed-off-by Dave Chinner <[email protected]>
I don't really see the real need for the wrapper, but it also doesn't seem actively harmful, so:
PATCH 3/3 xfs: add kmem_alloc_io()
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
Memory we use to submit for IO needs strict alignment to the underlying driver contraints. Worst case, this is 512 bytes. Given that all allocations for IO are always a power of 2 multiple of 512 bytes, the kernel heap provides natural alignment for objects of these sizes and that suffices.
Until, of course, memory debugging of some kind is turned on (e.g. red zones, poisoning, KASAN) and then the alignment of the heap objects is thrown out the window. Then we get weird IO errors and data corruption problems because drivers don't validate alignment and do the wrong thing when passed unaligned memory buffers in bios.
TO fix this, introduce kmem_alloc_io(), which will guaranteeat least 512 byte alignment of buffers for IO, even if memory debugging options are turned on. It is assumed that the minimum allocation size will be 512 bytes, and that sizes will be power of 2 mulitples of 512 bytes.
Use this everywhere we allocate buffers for IO.
This no longer fails with log recovery errors when KASAN is enabled due to the brd driver not handling unaligned memory buffers:
# mkfs.xfs -f /dev/ram0 ; mount /dev/ram0 /mnt/test
--- fs/xfs/kmem.c | 66 +++++++++++++++++++++++++++++----------- fs/xfs/kmem.h | 1 + fs/xfs/xfs_buf.c | 5 +-- fs/xfs/xfs_log.c | 5 +-- fs/xfs/xfs_log_recover.c | 4 ++- fs/xfs/xfs_trace.h | 1 + 6 files changed, 60 insertions(+), 22 deletions(-)
fs/xfs/kmem.c +49 -17
@@ -30,30 +30,24 @@ kmem_alloc(size_t size, xfs_km_flags_t flags) }while (1 ); } -void * -kmem_alloc_large (size_t size,xfs_km_flags_t flags) + +/* +* __vmalloc() will allocate data pages and auxillary structures (e.g. +* pagetables) with GFP_KERNEL, yet we may be under GFP_NOFS context here. Hence +* we need to tell memory reclaim that we are in such a context via +* PF_MEMALLOC_NOFS to prevent memory reclaim re-entering the filesystem here +* and potentially deadlocking. +*/ +static void * +__kmem_vmalloc (size_t size,xfs_km_flags_t flags) {unsigned nofs_flag =0 ;void *ptr; -gfp_t lflags; - -trace_kmem_alloc_large (size, flags, _RET_IP_); - - ptr =kmem_alloc (size, flags |KM_MAYFAIL ); -if (ptr) -return ptr; +gfp_t lflags =kmem_flags_convert (flags); -/* -* __vmalloc() will allocate data pages and auxillary structures (e.g. -* pagetables) with GFP_KERNEL, yet we may be under GFP_NOFS context -* here. Hence we need to tell memory reclaim that we are in such a -* context via PF_MEMALLOC_NOFS to prevent memory reclaim re-entering -* the filesystem here and potentially deadlocking. -*/ if (flags &KM_NOFS ) nofs_flag =memalloc_nofs_save (); - lflags =kmem_flags_convert (flags); ptr =__vmalloc (size, lflags,PAGE_KERNEL );if (flags &KM_NOFS )
@@ -62,6 +56,44 @@ kmem_alloc_large(size_t size, xfs_km_flags_t flags)return ptr; } +/* +* Same as kmem_alloc_large, except we guarantee the buffer returned is aligned +* to the @align_mask. We only guarantee alignment up to page size, we'll clamp +* alignment at page size if it is larger. vmalloc always returns a PAGE_SIZE +* aligned region. +*/ +void * +kmem_alloc_io (size_t size,int align_mask,xfs_km_flags_t flags) +{ +void *ptr; + +trace_kmem_alloc_io (size, flags, _RET_IP_); + +if (WARN_ON_ONCE (align_mask >=PAGE_SIZE )) + align_mask =PAGE_SIZE -1 ; + + ptr =kmem_alloc (size, flags |KM_MAYFAIL ); +if (ptr) { +if (!((uintptr_t )ptr & align_mask)) +return ptr; +kfree (ptr); + } +return __kmem_vmalloc (size, flags); +} + +void * +kmem_alloc_large (size_t size,xfs_km_flags_t flags) +{ +void *ptr; + +trace_kmem_alloc_large (size, flags, _RET_IP_); + + ptr =kmem_alloc (size, flags |KM_MAYFAIL ); +if (ptr) +return ptr; +return __kmem_vmalloc (size, flags); +} +void *kmem_realloc (const void *old,size_t newsize,xfs_km_flags_t flags) {
fs/xfs/kmem.h +1 -0
@@ -59,6 +59,7 @@ kmem_flags_convert(xfs_km_flags_t flags) }extern void *kmem_alloc (size_t ,xfs_km_flags_t ); +extern void *kmem_alloc_io (size_t size,int align_mask,xfs_km_flags_t flags);extern void *kmem_alloc_large (size_t size,xfs_km_flags_t );extern void *kmem_realloc (const void *,size_t ,xfs_km_flags_t );static inline void kmem_free (const void *ptr)
fs/xfs/xfs_buf.c +3 -2
@@ -353,7 +353,8 @@ xfs_buf_allocate_memory(*/ size =BBTOB (bp->b_length);if (size <PAGE_SIZE ) { - bp->b_addr =kmem_alloc (size,KM_NOFS ); +int align_mask =xfs_buftarg_dma_alignment (bp->b_target); + bp->b_addr =kmem_alloc_io (size, align_mask,KM_NOFS );if (!bp->b_addr) {/* low memory - use alloc_page loop instead */ goto use_alloc_page;
@@ -368,7 +369,7 @@ xfs_buf_allocate_memory( } bp->b_offset =offset_in_page (bp->b_addr); bp->b_pages = bp->b_page_array; - bp->b_pages[0 ] =virt_to_page (bp->b_addr); + bp->b_pages[0 ] =kmem_to_page (bp->b_addr); bp->b_page_count =1 ; bp->b_flags |= _XBF_KMEM;return 0 ;
fs/xfs/xfs_log.c +3 -2
@@ -1404,6 +1404,7 @@ xlog_alloc_log(*/ ASSERT (log->l_iclog_size >=4096 );for (i =0 ; i < log->l_iclog_bufs; i++) { +int align_mask =xfs_buftarg_dma_alignment (mp->m_logdev_targp);size_t bvec_size =howmany (log->l_iclog_size,PAGE_SIZE ) *sizeof (struct bio_vec );
@@ -1415,8 +1416,8 @@ xlog_alloc_log( iclog->ic_prev = prev_iclog; prev_iclog = iclog; - iclog->ic_data =kmem_alloc_large (log->l_iclog_size, -KM_MAYFAIL ); + iclog->ic_data =kmem_alloc_io (log->l_iclog_size, align_mask, +KM_MAYFAIL );if (!iclog->ic_data) goto out_free_iclog;#ifdef DEBUG
fs/xfs/xfs_log_recover.c +3 -1
@@ -97,6 +97,8 @@ xlog_alloc_buffer(struct xlog *log, int nbblks) { + int align_mask = xfs_buftarg_dma_alignment(log->l_targ); +/* * Pass log block 0 since we don't have an addr yet, buffer will be * verified on read.
@@ -125,7 +127,7 @@ xlog_alloc_buffer(if (nbblks >1 && log->l_sectBBsize >1 ) nbblks += log->l_sectBBsize; nbblks =round_up (nbblks, log->l_sectBBsize); -return kmem_alloc_large (BBTOB (nbblks),KM_MAYFAIL ); +return kmem_alloc_io (BBTOB (nbblks), align_mask,KM_MAYFAIL ); }/*
fs/xfs/xfs_trace.h +1 -0
@@ -3604,6 +3604,7 @@ DEFINE_EVENT(xfs_kmem_class, name, \ TP_PROTO(ssize_t size, int flags, unsigned long caller_ip), \ TP_ARGS(size, flags, caller_ip)) DEFINE_KMEM_EVENT(kmem_alloc); +DEFINE_KMEM_EVENT (kmem_alloc_io);DEFINE_KMEM_EVENT (kmem_alloc_large);DEFINE_KMEM_EVENT (kmem_realloc);DEFINE_KMEM_EVENT (kmem_zone_alloc);
Signature · 1 line
2.23.0.rc1
Re: [PATCH 3/3] xfs: add kmem_alloc_io()
fs/xfs/kmem.c kmem_alloc_large(size_t size, xfs_km_flags_t flags) · line 81
Quoted 7 lines
fs/xfs/kmem.c +7
-0+ ptr =kmem_alloc (size, flags |KM_MAYFAIL ); +if (ptr) { +if (!((uintptr_t )ptr & align_mask)) +return ptr; +kfree (ptr); + } +return __kmem_vmalloc (size, flags);
Not that іt really matters, I'd write so that the fast path is the main return:
ptr = kmem_alloc(size, flags | KM_MAYFAIL);
if (unlikely(!ptr || (intptr_t)ptr & align_mask)) {
kfree(ptr);
ptr = __kmem_vmalloc(size, flags);
}
return ptr;fs/xfs/xfs_buf.c xfs_buf_allocate_memory( · line 357
Quoted 2 lines
fs/xfs/xfs_buf.c +2
-0+int align_mask =xfs_buftarg_dma_alignment (bp->b_target); + bp->b_addr =kmem_alloc_io (size, align_mask,KM_NOFS );
The int vs an unsigned type here looks a little odd, but I guess that just propagates from queue_dma_alignment.
Otherwise looks good: