XFS realtime device tweaks
v4 cover XFS realtime device tweaks
Re-worked patch-set based on prior feedback & help from Dave Chinner. See change log in each patch for change details.
All code (and setting of sysfs options) in these patches should be safely gated behind XFS_IS_REALTIME_* macros. (Please point out any such cases which are not!)
Richard Wareing (3):
xfs: Show realtime device stats on statfs calls if inherit flag set
xfs: Set realtime flag based on initial allocation size
xfs: Add realtime fallback if data device full
fs/xfs/xfs_bmap_util.c | 5 ++++ fs/xfs/xfs_fsops.c | 4 +++ fs/xfs/xfs_inode.c | 18 ++++++++---- fs/xfs/xfs_iomap.c | 12 ++++++++ fs/xfs/xfs_linux.h | 2 ++ fs/xfs/xfs_mount.c | 27 ++++++++++++++++- fs/xfs/xfs_mount.h | 6 ++++ fs/xfs/xfs_rtalloc.c | 40 +++++++++++++++++++++++++ fs/xfs/xfs_rtalloc.h | 5 ++++ fs/xfs/xfs_super.c | 8 +++++ fs/xfs/xfs_sysfs.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++ 11 files changed, 200 insertions(+), 7 deletions(-)
Signature · 1 line
2.9.5
PATCH v4 1/3 xfs: Show realtime device stats on statfs calls if inherit flag set
- Reports realtime device free blocks in statfs calls if inheritance bit is set on the inode of directory. This is a bit more intuitive, especially for use-cases which are using a much larger device for the realtime device.
- Add XFS_IS_REALTIME_MOUNT option to gate based on the existence of a realtime device on the mount, similar to the XFS_IS_REALTIME_INODE option.
---
Changes since v3:
* Fixed accounting bug, we are not required to substract m_alloc_set_aside as this is a data device only requirement.
* Added XFS_IS_REALTIME_MOUNT macro based on learnings from CVE-2017-14340, now provides similar gating on the mount as XFS_IS_REALTIME_INODE does for the inode.
Changes since v2:
* Style updated per Christoph Hellwig's comment
* Fixed bug: statp->f_bavail = statp->f_bfree
fs/xfs/xfs_linux.h | 2 ++ fs/xfs/xfs_super.c | 8 ++++++++ 2 files changed, 10 insertions(+)
fs/xfs/xfs_linux.h +2 -0
@@ -280,8 +280,10 @@ static inline __uint64_t howmany_64(__uint64_t x, __uint32_t y)#ifdef CONFIG_XFS_RT #define XFS_IS_REALTIME_INODE (ip) ((ip)->i_d.di_flags & XFS_DIFLAG_REALTIME) +#define XFS_IS_REALTIME_MOUNT (mp) ((mp)->m_rtdev_targp ? 1 : 0)#else #define XFS_IS_REALTIME_INODE (ip) (0) +#define XFS_IS_REALTIME_MOUNT (mp) (0)#endif #endif /* __XFS_LINUX__ */
fs/xfs/xfs_super.c +8 -0
@@ -1136,6 +1136,14 @@ xfs_fs_statfs( ((mp->m_qflags & (XFS_PQUOTA_ACCT|XFS_PQUOTA_ENFD))) == (XFS_PQUOTA_ACCT|XFS_PQUOTA_ENFD))xfs_qm_statvfs (ip, statp); + +if (XFS_IS_REALTIME_MOUNT (mp) && + (ip->i_d.di_flags &XFS_DIFLAG_RTINHERIT )) { + statp->f_blocks = sbp->sb_rblocks; + statp->f_bavail = statp->f_bfree = + sbp->sb_frextents * sbp->sb_rextsize; + } +return 0 ; }
Signature · 1 line
2.9.5
Re: [PATCH v4 1/3] xfs: Show realtime device stats on statfs calls if inherit flag set
Quoted 9 lines · Richard Wareing
- Reports realtime device free blocks in statfs calls if inheritance bit is set on the inode of directory. This is a bit more intuitive, especially for use-cases which are using a much larger device for the realtime device.
- AddXFS_IS_REALTIME_MOUNToption to gate based on the existence of a realtime device on the mount, similar to theXFS_IS_REALTIME_INODEoption.Signed-off-by Richard Wareing <[email protected]>
Didn't Christoph give a Reviewed-by for this one? Normally when we repost a patch that already has a revewied-by we include it in the commit message. That tells other reviewers and the maintainer that it has already been reviewed :P
Quoted 20 lines
Changes since v3:
* Fixed accounting bug, we are not required to substractm_alloc_set_asideas this is a data device only requirement.
* AddedXFS_IS_REALTIME_MOUNTmacro based on learnings from CVE-2017-14340, now provides similar gating on the mount asXFS_IS_REALTIME_INODEdoes for the inode.Changes since v2:
* Style updated per Christoph Hellwig's comment
* Fixed bug:statp->f_bavail=statp->f_bfreefs/xfs/xfs_linux.h | 2 ++ fs/xfs/xfs_super.c | 8 ++++++++ 2 files changed, 10 insertions(+)fs/xfs/xfs_linux.h +0
-0
fs/xfs/xfs_linux.h static inline __uint64_t howmany_64(__uint64_t x, __uint32_t y) · line 287
Quoted 8 lines
fs/xfs/xfs_linux.h +2
-0@@ -280,8 +280,10 @@ static inline __uint64_t howmany_64(__uint64_t x, __uint32_t y)#ifdef CONFIG_XFS_RT #define XFS_IS_REALTIME_INODE (ip) ((ip)->i_d.di_flags & XFS_DIFLAG_REALTIME) +#define XFS_IS_REALTIME_MOUNT (mp) ((mp)->m_rtdev_targp ? 1 : 0)#else #define XFS_IS_REALTIME_INODE (ip) (0) +#define XFS_IS_REALTIME_MOUNT (mp) (0)#endif
Good idea. I think we should probably turn these into inline functions. That can be done in a new patch, though.
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH v4 1/3] xfs: Show realtime device stats on statfs calls if inherit flag set
Quoted 9 lines · Dave Chinner
Quoted 8 lines · Richard Wareing
- Reports realtime device free blocks in statfs calls if inheritance bit is set on the inode of directory. This is a bit more intuitive, especially for use-cases which are using a much larger device for the realtime device.
- AddXFS_IS_REALTIME_MOUNToption to gate based on the existence of a realtime device on the mount, similar to theXFS_IS_REALTIME_INODEoption.
Quoted 7 lines
Quoted 1 line
Signed-off-by Richard Wareing <[email protected]>Didn't Christoph give a Reviewed-by for this one? Normally when we repost a patch that already has a revewied-by we include it in the commit message. That tells other reviewers and the maintainer that it has already been reviewed :P
Oh, I completely missed it (back on Sept. 3rd). I'd rather go with this version with the "XFS_IS_REALTIME_MOUNT" addition given the learnings from the CVE.
I'll be sure to mention it in my commit message in the future.
Thanks,
Richard
Quoted 41 lines
Quoted 20 lines
Changes since v3:
* Fixed accounting bug, we are not required to substractm_alloc_set_asideas this is a data device only requirement.
* AddedXFS_IS_REALTIME_MOUNTmacro based on learnings from CVE-2017-14340, now provides similar gating on the mount asXFS_IS_REALTIME_INODEdoes for the inode.Changes since v2:
* Style updated per Christoph Hellwig's comment
* Fixed bug:statp->f_bavail=statp->f_bfreefs/xfs/xfs_linux.h | 2 ++ fs/xfs/xfs_super.c | 8 ++++++++ 2 files changed, 10 insertions(+)fs/xfs/xfs_linux.h +0
-0fs/xfs/xfs_linux.h static inline __uint64_t howmany_64(__uint64_t x, · line 288
Quoted 9 lines
fs/xfs/xfs_linux.h +2
-0@@ -280,8 +280,10 @@ static inline __uint64_t howmany_64(__uint64_t x, __uint32_t y)#ifdef CONFIG_XFS_RT #define XFS_IS_REALTIME_INODE (ip) ((ip)->i_d.di_flags & XFS_DIFLAG_REALTIME) +#define XFS_IS_REALTIME_MOUNT (mp) ((mp)->m_rtdev_targp ? 1 : 0)#else #define XFS_IS_REALTIME_INODE (ip) (0) +#define XFS_IS_REALTIME_MOUNT (mp) (0)#endif Good idea. I think we should probably turn these into inline functions. That can be done in a new patch, though.
Reviewed-by Dave Chinner <[email protected]>Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
PATCH v4 2/3 xfs: Set realtime flag based on initial allocation size
In reply to Richard Wareing- The rt_alloc_min sysfs option automatically selects the device (data device, or realtime) based on the size of the initial allocation of the file.
- This option can be used to route the storage of small files (and the inefficient workloads associated with them) to a suitable storage device such a SSD, while larger allocations are sent to a traditional HDD.
- Supports writes via O_DIRECT, buffered (i.e. page cache), and pre-allocations (i.e. fallocate)
- Available only when kernel is compiled w/ CONFIG_XFS_RT option.
---
Changes since v3:
* Now functions via initial allocation regardless of O_DIRECT, buffered or pre-allocation code paths. Provides a consistent user-experience.
* I Did do some experiments putting this in the xfs_bmapi_write code path however pre-allocation accounting unfortunately prevents this cleaner approach. As such, this proved to be the cleanest and functional approach.
* No longer a mount option, now a sysfs tunable
Changes since v2:
* None
fs/xfs/xfs_bmap_util.c | 3 +++ fs/xfs/xfs_inode.c | 18 ++++++++++++------ fs/xfs/xfs_iomap.c | 8 ++++++++ fs/xfs/xfs_mount.h | 1 + fs/xfs/xfs_rtalloc.c | 26 ++++++++++++++++++++++++++ fs/xfs/xfs_rtalloc.h | 4 ++++ fs/xfs/xfs_sysfs.c | 41 +++++++++++++++++++++++++++++++++++++++++ 7 files changed, 95 insertions(+), 6 deletions(-)
fs/xfs/xfs_bmap_util.c +3 -0
@@ -1026,6 +1026,9 @@ xfs_alloc_file_space(if (len <=0 )return -EINVAL ; +if (XFS_IS_REALTIME_MOUNT (mp)) +xfs_rt_alloc_min (ip, len); + rt =XFS_IS_REALTIME_INODE (ip); extsz =xfs_get_extsz_hint (ip);
fs/xfs/xfs_inode.c +12 -6
@@ -1620,12 +1620,18 @@ xfs_itruncate_extents(if (error) goto out; -/* -* Clear the reflink flag if we truncated everything. -*/ -if (ip->i_d.di_nblocks ==0 &&xfs_is_reflink_inode (ip)) { - ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK ; -xfs_inode_clear_cowblocks_tag (ip); +if (ip->i_d.di_nblocks ==0 ) { +/* +* Clear the reflink flag if we truncated everything. +*/ +if (xfs_is_reflink_inode (ip)) { + ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK ; +xfs_inode_clear_cowblocks_tag (ip); + } +/* Clear realtime flag if m_rt_alloc_min policy is in place */ +if (XFS_IS_REALTIME_MOUNT (mp) && mp->m_rt_alloc_min) { + ip->i_d.di_flags &= ~XFS_DIFLAG_REALTIME ; + } }/*
fs/xfs/xfs_iomap.c +8 -0
@@ -40,6 +40,7 @@#include "xfs_dquot_item.h" #include "xfs_dquot.h" #include "xfs_reflink.h" +#include "xfs_rtalloc.h" #define XFS_WRITEIO_ALIGN (mp,off) (((off) >> mp->m_writeio_log) \
@@ -174,6 +175,10 @@ xfs_iomap_write_direct(int bmapi_flags =XFS_BMAPI_PREALLOC ;uint tflags =0 ; + +if (XFS_IS_REALTIME_MOUNT (mp)) +xfs_rt_alloc_min (ip, count); + rt =XFS_IS_REALTIME_INODE (ip); extsz =xfs_get_extsz_hint (ip); lockmode =XFS_ILOCK_SHARED ;/* locked by caller */
@@ -981,6 +986,9 @@ xfs_file_iomap_begin(if (XFS_FORCED_SHUTDOWN (mp))return -EIO ; +if (XFS_IS_REALTIME_MOUNT (mp)) +xfs_rt_alloc_min (ip, length); +if (((flags & (IOMAP_WRITE |IOMAP_DIRECT )) ==IOMAP_WRITE ) && !IS_DAX (inode) && !xfs_get_extsz_hint (ip)) {/* Reserve delalloc blocks for regular writeback. */
fs/xfs/xfs_mount.h +1 -0
@@ -197,6 +197,7 @@ typedef struct xfs_mount {__uint32_t m_generation;bool m_fail_unmount; +uint m_rt_alloc_min;/* Min RT allocation */ #ifdef DEBUG /* * DEBUG mode instrumentation to test and/or trigger delayed allocation
fs/xfs/xfs_rtalloc.c +26 -0
@@ -1284,3 +1284,29 @@ xfs_rtpick_extent( *pick = b;return 0 ; } + +/* +* Automatically set real-time flag if initial write to inode is > m_rt_alloc_min +* +* Also valid on truncations. +* +*/ +void xfs_rt_alloc_min ( +struct xfs_inode *ip, +xfs_off_t len) +{ +struct xfs_mount *mp = ip->i_mount; + +if (!mp->m_rt_alloc_min || ip->i_d.di_size) +return ; + +if (XFS_IS_REALTIME_INODE (ip)) { +if (len < mp->m_rt_alloc_min) { + ip->i_d.di_flags &= ~XFS_DIFLAG_REALTIME ; + } + }else { +if (len >= mp->m_rt_alloc_min) { + ip->i_d.di_flags |=XFS_DIFLAG_REALTIME ; + } + } +}
fs/xfs/xfs_rtalloc.h +4 -0
@@ -136,6 +136,9 @@ int xfs_rtalloc_query_range(struct xfs_trans *tp,int xfs_rtalloc_query_all (struct xfs_trans *tp,xfs_rtalloc_query_range_fn fn,void *priv); +void xfs_rt_alloc_min (struct xfs_inode *ip,xfs_off_t len); + +#else # define xfs_rtallocate_extent (t,b,min,max,l,f,p,rb) (ENOSYS)# define xfs_rtfree_extent (t,b,l) (ENOSYS)
@@ -155,6 +158,7 @@ xfs_rtmount_init( }# define xfs_rtmount_inodes (m) (((mp)->m_sb.sb_rblocks == 0)? 0 : (ENOSYS))# define xfs_rtunmount_inodes (m) +# define xfs_rt_alloc_min (i,l) (ENOSYS)#endif /* CONFIG_XFS_RT */ #endif /* __XFS_RTALLOC_H__ */
fs/xfs/xfs_sysfs.c +41 -0
51 lines not on this page · raw message
Signature · 1 line
2.9.5
Re: [PATCH v4 2/3] xfs: Set realtime flag based on initial allocation size
Quoted 38 lines · Richard Wareing
- The
rt_alloc_minsysfs option automatically selects the device (data device, or realtime) based on the size of the initial allocation of the file.
- This option can be used to route the storage of small files (and the inefficient workloads associated with them) to a suitable storage device such a SSD, while larger allocations are sent to a traditional HDD.
- Supports writes viaO_DIRECT, buffered (i.e. page cache), and pre-allocations (i.e. fallocate)
- Available only when kernel is compiled w/CONFIG_XFS_RToption.Signed-off-by Richard Wareing <[email protected]>---
Changes since v3:
* Now functions via initial allocation regardless ofO_DIRECT, buffered or pre-allocation code paths. Provides a consistent user-experience.
* I Did do some experiments putting this in thexfs_bmapi_writecode path however pre-allocation accounting unfortunately prevents this cleaner approach. As such, this proved to be the cleanest and functional approach.
* No longer a mount option, now a sysfs tunableChanges since v2:
* Nonefs/xfs/xfs_bmap_util.c | 3 +++ fs/xfs/xfs_inode.c | 18 ++++++++++++------ fs/xfs/xfs_iomap.c | 8 ++++++++ fs/xfs/xfs_mount.h | 1 + fs/xfs/xfs_rtalloc.c | 26 ++++++++++++++++++++++++++ fs/xfs/xfs_rtalloc.h | 4 ++++ fs/xfs/xfs_sysfs.c | 41 +++++++++++++++++++++++++++++++++++++++++ 7 files changed, 95 insertions(+), 6 deletions(-)fs/xfs/xfs_bmap_util.c +0
-0
fs/xfs/xfs_bmap_util.c xfs_alloc_file_space( · line 1030
Quoted 5 lines
fs/xfs/xfs_bmap_util.c +2
-0@@ -1026,6 +1026,9 @@ xfs_alloc_file_space(if (len <=0 )return -EINVAL ; +if (XFS_IS_REALTIME_MOUNT (mp)) +xfs_rt_alloc_min (ip, len);
I'd put the XFS_IS_REALTIME_MOUNT() check inside xfs_rt_alloc_min(). That way we can compile the code out completely when CONFIG_XFS_RT=n.
Quoted 17 lines
fs/xfs/xfs_bmap_util.c +0
-0rt =XFS_IS_REALTIME_INODE (ip); extsz =xfs_get_extsz_hint (ip);fs/xfs/xfs_inode.c +0
-6@@ -1620,12 +1620,18 @@ xfs_itruncate_extents(if (error) goto out; -/* -* Clear the reflink flag if we truncated everything. -*/ -if (ip->i_d.di_nblocks ==0 &&xfs_is_reflink_inode (ip)) { - ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK ; -xfs_inode_clear_cowblocks_tag (ip);
fs/xfs/xfs_inode.c xfs_itruncate_extents( · line 1632
Quoted 10 lines
fs/xfs/xfs_inode.c +10
-0+if (ip->i_d.di_nblocks ==0 ) { +/* +* Clear the reflink flag if we truncated everything. +*/ +if (xfs_is_reflink_inode (ip)) { + ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK ; +xfs_inode_clear_cowblocks_tag (ip); + } +/* Clear realtime flag if m_rt_alloc_min policy is in place */ +if (XFS_IS_REALTIME_MOUNT (mp) && mp->m_rt_alloc_min) {
Won't m_rt_alloc_min always be zero on non-rt filesystems?
Quoted 18 lines
fs/xfs/xfs_inode.c +2
-0+ ip->i_d.di_flags &= ~XFS_DIFLAG_REALTIME ; + } }/* fs/xfs/xfs_iomap.c +1
-0@@ -40,6 +40,7 @@#include "xfs_dquot_item.h" #include "xfs_dquot.h" #include "xfs_reflink.h" +#include "xfs_rtalloc.h" #define XFS_WRITEIO_ALIGN (mp,off) (((off) >> mp->m_writeio_log) \
fs/xfs/xfs_iomap.c xfs_iomap_write_direct( · line 180
Quoted 6 lines
fs/xfs/xfs_iomap.c +3
-0@@ -174,6 +175,10 @@ xfs_iomap_write_direct(int bmapi_flags =XFS_BMAPI_PREALLOC ;uint tflags =0 ; + +if (XFS_IS_REALTIME_MOUNT (mp)) +xfs_rt_alloc_min (ip, count);
Reading this makes me wonder what we are allocating here :/ A better name might be in order - something that indicates we're selecting the target device rather allocating something. e.g. xfs_inode_select_target()?
fs/xfs/xfs_mount.h typedef struct xfs_mount { · line 200
Quoted 9 lines
fs/xfs/xfs_mount.h +1
-0@@ -197,6 +197,7 @@ typedef struct xfs_mount {__uint32_t m_generation;bool m_fail_unmount; +uint m_rt_alloc_min;/* Min RT allocation */
WHitespace problem - looks like spaces instead of tabs....
Quoted 16 lines
fs/xfs/xfs_mount.h +0
-0#ifdef DEBUG /* * DEBUG mode instrumentation to test and/or trigger delayed allocation fs/xfs/xfs_rtalloc.c +5
-0@@ -1284,3 +1284,29 @@ xfs_rtpick_extent( *pick = b;return 0 ; } + +/* +* Automatically set real-time flag if initial write to inode is > m_rt_alloc_min +* +* Also valid on truncations.
fs/xfs/xfs_rtalloc.c xfs_rtpick_extent( · line 1301
Quoted 10 lines
fs/xfs/xfs_rtalloc.c +10
-0+* +*/ +void xfs_rt_alloc_min ( +struct xfs_inode *ip, +xfs_off_t len) +{ +struct xfs_mount *mp = ip->i_mount; + +if (!mp->m_rt_alloc_min || ip->i_d.di_size) +return ;
I kinda prefer stacking single checks like
/* * m_rt_alloc_min controls the target selection. It is * inactive if it is not set. */ if (!mp->m_rt_alloc_min) return; /* * Can't select a different target if we've already * allocated blocks (e.g. fallocate() beyond EOF) or has * data in it already. */ if (!ip->i_nextents) return; if (!ip->i_d.di_size) return;
fs/xfs/xfs_rtalloc.c xfs_rtpick_extent( · line 1311
Quoted 9 lines
fs/xfs/xfs_rtalloc.c +9
-0+if (XFS_IS_REALTIME_INODE (ip)) { +if (len < mp->m_rt_alloc_min) { + ip->i_d.di_flags &= ~XFS_DIFLAG_REALTIME ; + } + }else { +if (len >= mp->m_rt_alloc_min) { + ip->i_d.di_flags |=XFS_DIFLAG_REALTIME ; + } + }
Checking for XFS_IS_REALTIME_INODE() is redundant here. This does the same thing:
/* * if the allocation length is less than the threshold, * always select the data device. Otherwise we should * select the realtime device. */ if (len < mp->m_rt_alloc_min) ip->i_d.di_flags &= ~XFS_DIFLAG_REALTIME; else ip->i_d.di_flags |= XFS_DIFLAG_REALTIME;
Quoted 43 lines
fs/xfs/xfs_rtalloc.c +1
-0+}fs/xfs/xfs_rtalloc.h +4
-0@@ -136,6 +136,9 @@ int xfs_rtalloc_query_range(struct xfs_trans *tp,int xfs_rtalloc_query_all (struct xfs_trans *tp,xfs_rtalloc_query_range_fn fn,void *priv); +void xfs_rt_alloc_min (struct xfs_inode *ip,xfs_off_t len); + +#else # define xfs_rtallocate_extent (t,b,min,max,l,f,p,rb) (ENOSYS)# define xfs_rtfree_extent (t,b,l) (ENOSYS)@@ -155,6 +158,7 @@ xfs_rtmount_init( }# define xfs_rtmount_inodes (m) (((mp)->m_sb.sb_rblocks == 0)? 0 : (ENOSYS))# define xfs_rtunmount_inodes (m) +# define xfs_rt_alloc_min (i,l) (ENOSYS)#endif /* CONFIG_XFS_RT */ #endif /* __XFS_RTALLOC_H__ */ fs/xfs/xfs_sysfs.c +12
-0@@ -129,10 +129,51 @@ XFS_SYSFS_ATTR_RW(drop_writes);#endif /* DEBUG */ +#ifdef CONFIG_XFS_RT +STATIC ssize_t +rt_alloc_min_store ( +struct kobject *kobject, +const char *buf, +size_t count) +{ +struct xfs_mount *mp =to_mp (kobject); +int ret; +int val; + + ret =kstrtoint (buf,0 , &val);
fs/xfs/xfs_sysfs.c XFS_SYSFS_ATTR_RW(drop_writes); · line 153
Quoted 10 lines
fs/xfs/xfs_sysfs.c +10
-0+if (ret) +return ret; + +/* Only valid if using a real-time device */ +if (XFS_IS_REALTIME_MOUNT (mp) && val >0 ) + mp->m_rt_alloc_min = val; +else if (val <=0 ) + mp->m_rt_alloc_min =0 ; +else +return -EINVAL ;
Seems inconsistent. This will allow a value <= 0 for a non-realtime mount, but will return EINVAL for values > 0. Perhaps it would be more consistent to return EINVAL for a non-realtime mount or any value < 0?
Quoted 13 lines
fs/xfs/xfs_sysfs.c +13
-0+ +return count; +} + +STATIC ssize_t +rt_alloc_min_show ( +struct kobject *kobject, +char *buf) +{ +struct xfs_mount *mp =to_mp (kobject); + +return snprintf (buf,PAGE_SIZE ,"%d\n" , mp->m_rt_alloc_min); +}
fs/xfs/xfs_sysfs.c XFS_SYSFS_ATTR_RW(drop_writes); · line 176
Quoted 10 lines
fs/xfs/xfs_sysfs.c +6
-0+XFS_SYSFS_ATTR_RW (rt_alloc_min); +#endif /* CONFIG_XFS_RT */ + static struct attribute *xfs_mp_attrs[] = {#ifdef DEBUG ATTR_LIST(drop_writes),#endif +#ifdef CONFIG_XFS_RT + ATTR_LIST(rt_alloc_min), +#endif
This is userspace visible - shouldn't we always compile this in, even if all it does it return EINVAL to attempts to change it when CONFIG_XFS_RT=n?
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH v4 2/3] xfs: Set realtime flag based on initial allocation size
Quoted 42 lines · Dave Chinner
Quoted 41 lines · Richard Wareing
- The
rt_alloc_minsysfs option automatically selects the device (data device, or realtime) based on the size of the initial allocation of the file.
- This option can be used to route the storage of small files (and the inefficient workloads associated with them) to a suitable storage device such a SSD, while larger allocations are sent to a traditional HDD.
- Supports writes viaO_DIRECT, buffered (i.e. page cache), and pre-allocations (i.e. fallocate)
- Available only when kernel is compiled w/CONFIG_XFS_RToption.Signed-off-by Richard Wareing <[email protected]>---
Changes since v3:
* Now functions via initial allocation regardless ofO_DIRECT, buffered or pre-allocation code paths. Provides a consistent user-experience.
* I Did do some experiments putting this in thexfs_bmapi_writecode path however pre-allocation accounting unfortunately prevents this cleaner approach. As such, this proved to be the cleanest and functional approach.
* No longer a mount option, now a sysfs tunableChanges since v2:
* Nonefs/xfs/xfs_bmap_util.c | 3 +++ fs/xfs/xfs_inode.c | 18 ++++++++++++------ fs/xfs/xfs_iomap.c | 8 ++++++++ fs/xfs/xfs_mount.h | 1 + fs/xfs/xfs_rtalloc.c | 26 ++++++++++++++++++++++++++ fs/xfs/xfs_rtalloc.h | 4 ++++ fs/xfs/xfs_sysfs.c | 41 +++++++++++++++++++++++++++++++++++++++++ 7 files changed, 95 insertions(+), 6 deletions(-)fs/xfs/xfs_bmap_util.c +0
-0@@ -1026,6 +1026,9 @@ xfs_alloc_file_space(if (len <=0 )return -EINVAL ;
Quoted 7 lines
fs/xfs/xfs_bmap_util.c xfs_alloc_file_space( · line 1030
Quoted 2 lines
fs/xfs/xfs_bmap_util.c +2
-0+if (XFS_IS_REALTIME_MOUNT (mp)) +xfs_rt_alloc_min (ip, len);I'd put the
XFS_IS_REALTIME_MOUNT()check insidexfs_rt_alloc_min(). That way we can compile the code out completely whenCONFIG_XFS_RT=n.
I was wondering about this, but elected to model the XFS_IS_REALTIME_INODE code structure. I'm partial to putting this in the function as well, as it reads cleaner too.
Quoted 21 lines
Quoted 21 lines
fs/xfs/xfs_bmap_util.c +0
-0rt =XFS_IS_REALTIME_INODE (ip); extsz =xfs_get_extsz_hint (ip);fs/xfs/xfs_inode.c +4
-6@@ -1620,12 +1620,18 @@ xfs_itruncate_extents(if (error) goto out; -/* -* Clear the reflink flag if we truncated everything. -*/ -if (ip->i_d.di_nblocks ==0 &&xfs_is_reflink_inode (ip)) { - ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK ; -xfs_inode_clear_cowblocks_tag (ip); +if (ip->i_d.di_nblocks ==0 ) { +/* +* Clear the reflink flag if we truncated everything. +*/
Quoted 9 lines
fs/xfs/xfs_inode.c xfs_itruncate_extents( · line 1632
Quoted 6 lines
fs/xfs/xfs_inode.c +6
-0+if (xfs_is_reflink_inode (ip)) { + ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK ; +xfs_inode_clear_cowblocks_tag (ip); + } +/* Clear realtime flag if m_rt_alloc_min policy is in place */ +if (XFS_IS_REALTIME_MOUNT (mp) && mp->m_rt_alloc_min) {Won't
m_rt_alloc_minalways be zero on non-rt filesystems?
Yes, but was trying to be defensive, in case some future me or other developer screwed something up in the future :).
Quoted 21 lines
Quoted 21 lines
fs/xfs/xfs_inode.c +2
-0+ ip->i_d.di_flags &= ~XFS_DIFLAG_REALTIME ; + } }/* fs/xfs/xfs_iomap.c +1
-0@@ -40,6 +40,7 @@#include "xfs_dquot_item.h" #include "xfs_dquot.h" #include "xfs_reflink.h" +#include "xfs_rtalloc.h" #define XFS_WRITEIO_ALIGN (mp,off) (((off) >> mp->m_writeio_log) \@@ -174,6 +175,10 @@ xfs_iomap_write_direct(int bmapi_flags =XFS_BMAPI_PREALLOC ;uint tflags =0 ;
Quoted 8 lines
fs/xfs/xfs_iomap.c xfs_iomap_write_direct( · line 180
Quoted 3 lines
fs/xfs/xfs_iomap.c +3
-0+ +if (XFS_IS_REALTIME_MOUNT (mp)) +xfs_rt_alloc_min (ip, count);Reading this makes me wonder what we are allocating here :/ A better name might be in order - something that indicates we're selecting the target device rather allocating something. e.g.
xfs_inode_select_target()?
I like your function name better than mine, changing this.
Quoted 8 lines
Quoted 8 lines
fs/xfs/xfs_mount.h +0
-0@@ -197,6 +197,7 @@ typedef struct xfs_mount {__uint32_t m_generation;
Quoted 5 lines
fs/xfs/xfs_mount.h typedef struct xfs_mount { · line 200
Quoted 2 lines
fs/xfs/xfs_mount.h +1
-0bool m_fail_unmount; +uint m_rt_alloc_min;/* Min RT allocation */ WHitespace problem - looks like spaces instead of tabs....
Fixing.
Quoted 30 lines
Quoted 19 lines
fs/xfs/xfs_mount.h +0
-0#ifdef DEBUG /* * DEBUG mode instrumentation to test and/or trigger delayed allocation fs/xfs/xfs_rtalloc.c +3
-0@@ -1284,3 +1284,29 @@ xfs_rtpick_extent( *pick = b;return 0 ; } + +/* +* Automatically set real-time flag if initial write to inode is >
m_rt_alloc_min
+ *
+ * Also valid on truncations.
+ *
+ */fs/xfs/xfs_rtalloc.c xfs_rtpick_extent( · line 1301
Quoted 8 lines
fs/xfs/xfs_rtalloc.c +8
-0+void xfs_rt_alloc_min ( +struct xfs_inode *ip, +xfs_off_t len) +{ +struct xfs_mount *mp = ip->i_mount; + +if (!mp->m_rt_alloc_min || ip->i_d.di_size) +return ;I kinda prefer stacking single checks like
Quoted 17 lines
/* * m_rt_alloc_min controls the target selection. It is * inactive if it is not set. */ if (!mp->m_rt_alloc_min) return; /* * Can't select a different target if we've already * allocated blocks (e.g. fallocate() beyond EOF) or has * data in it already. */ if (!ip->i_nextents) return; if (!ip->i_d.di_size) return;
Fixing.
Quoted 13 lines
fs/xfs/xfs_rtalloc.c xfs_rtpick_extent( · line 1311
Quoted 9 lines
fs/xfs/xfs_rtalloc.c +9
-0+if (XFS_IS_REALTIME_INODE (ip)) { +if (len < mp->m_rt_alloc_min) { + ip->i_d.di_flags &= ~XFS_DIFLAG_REALTIME ; + } + }else { +if (len >= mp->m_rt_alloc_min) { + ip->i_d.di_flags |=XFS_DIFLAG_REALTIME ; + } + }Checking for
XFS_IS_REALTIME_INODE()is redundant here. This does the same thing:
Quoted 10 lines
/* * if the allocation length is less than the threshold, * always select the data device. Otherwise we should * select the realtime device. */ if (len < mp->m_rt_alloc_min) ip->i_d.di_flags &= ~XFS_DIFLAG_REALTIME; else ip->i_d.di_flags |= XFS_DIFLAG_REALTIME;
Fixing.
Quoted 50 lines
Quoted 50 lines
fs/xfs/xfs_rtalloc.c +1
-0+}fs/xfs/xfs_rtalloc.h +4
-0@@ -136,6 +136,9 @@ int xfs_rtalloc_query_range(struct xfs_trans *tp,int xfs_rtalloc_query_all (struct xfs_trans *tp,xfs_rtalloc_query_range_fn fn,void *priv); +void xfs_rt_alloc_min (struct xfs_inode *ip,xfs_off_t len); + +#else # define xfs_rtallocate_extent (t,b,min,max,l,f,p,rb) (ENOSYS)# define xfs_rtfree_extent (t,b,l) (ENOSYS)@@ -155,6 +158,7 @@ xfs_rtmount_init( }# define xfs_rtmount_inodes (m) (((mp)->m_sb.sb_rblocks == 0)? 0 : (ENOSYS))# define xfs_rtunmount_inodes (m) +# define xfs_rt_alloc_min (i,l) (ENOSYS)#endif /* CONFIG_XFS_RT */ #endif /* __XFS_RTALLOC_H__ */ fs/xfs/xfs_sysfs.c +19
-0@@ -129,10 +129,51 @@ XFS_SYSFS_ATTR_RW(drop_writes);#endif /* DEBUG */ +#ifdef CONFIG_XFS_RT +STATIC ssize_t +rt_alloc_min_store ( +struct kobject *kobject, +const char *buf, +size_t count) +{ +struct xfs_mount *mp =to_mp (kobject); +int ret; +int val; + + ret =kstrtoint (buf,0 , &val); +if (ret) +return ret; + +/* Only valid if using a real-time device */ +if (XFS_IS_REALTIME_MOUNT (mp) && val >0 ) + mp->m_rt_alloc_min = val; +else if (val <=0 )
Quoted 8 lines
fs/xfs/xfs_sysfs.c XFS_SYSFS_ATTR_RW(drop_writes); · line 153
Quoted 3 lines
fs/xfs/xfs_sysfs.c +3
-0+ mp->m_rt_alloc_min =0 ; + else + return -EINVAL;Seems inconsistent. This will allow a value <= 0 for a non-realtime mount, but will return EINVAL for values > 0. Perhaps it would be more consistent to return EINVAL for a non-realtime mount or any value < 0?
Fair observation, fixing.
Quoted 20 lines
Quoted 20 lines
fs/xfs/xfs_sysfs.c +16
-0+ +return count; +} + +STATIC ssize_t +rt_alloc_min_show ( +struct kobject *kobject, +char *buf) +{ +struct xfs_mount *mp =to_mp (kobject); + +return snprintf (buf,PAGE_SIZE ,"%d\n" , mp->m_rt_alloc_min); +} +XFS_SYSFS_ATTR_RW (rt_alloc_min); +#endif /* CONFIG_XFS_RT */ + static struct attribute *xfs_mp_attrs[] = {#ifdef DEBUG ATTR_LIST(drop_writes),
Quoted 9 lines
fs/xfs/xfs_sysfs.c XFS_SYSFS_ATTR_RW(drop_writes); · line 176
Quoted 4 lines
fs/xfs/xfs_sysfs.c +3
-0#endif +#ifdef CONFIG_XFS_RT + ATTR_LIST(rt_alloc_min), +#endif This is userspace visible - shouldn't we always compile this in, even if all it does it return EINVAL to attempts to change it when
CONFIG_XFS_RT=n?
I'm fine with changing this, my thinking here was to simply not show options which aren't tunable without a realtime device. But I'm fine with throwing EINVAL too.
Richard
PATCH v4 3/3 xfs: Add realtime fallback if data device full
In reply to Richard Wareing- Adds tunable option to fallback to realtime device if configured when data device is full.
- Useful for realtime device users to help prevent ENOSPC errors when selectively storing some files (e.g. small files) on data device, while others are stored on realtime block device.
- Set via the "rt_fallback_pct" sysfs value which is available if the kernel is compiled with CONFIG_XFS_RT.
---
Changes since v3:
* None, new patch to patch set
fs/xfs/xfs_bmap_util.c | 4 +++- fs/xfs/xfs_fsops.c | 4 ++++ fs/xfs/xfs_iomap.c | 8 ++++++-- fs/xfs/xfs_mount.c | 27 ++++++++++++++++++++++++++- fs/xfs/xfs_mount.h | 7 ++++++- fs/xfs/xfs_rtalloc.c | 14 ++++++++++++++ fs/xfs/xfs_rtalloc.h | 3 ++- fs/xfs/xfs_sysfs.c | 39 +++++++++++++++++++++++++++++++++++++++ 8 files changed, 100 insertions(+), 6 deletions(-)
fs/xfs/xfs_bmap_util.c +3 -1
@@ -1026,8 +1026,10 @@ xfs_alloc_file_space(if (len <=0 )return -EINVAL ; -if (XFS_IS_REALTIME_MOUNT (mp)) +if (XFS_IS_REALTIME_MOUNT (mp)) {xfs_rt_alloc_min (ip, len); +xfs_rt_fallback (ip, mp); + } rt =XFS_IS_REALTIME_INODE (ip); extsz =xfs_get_extsz_hint (ip);
fs/xfs/xfs_fsops.c +4 -0
@@ -610,6 +610,10 @@ xfs_growfs_data_private(xfs_set_low_space_thresholds (mp); mp->m_alloc_set_aside =xfs_alloc_set_aside (mp); +if (XFS_IS_REALTIME_MOUNT (mp)) { +xfs_set_rt_min_fdblocks (mp); + } +/* * If we expanded the last AG, free the per-AG reservation * so we can reinitialize it with the new size.
fs/xfs/xfs_iomap.c +6 -2
@@ -176,8 +176,10 @@ xfs_iomap_write_direct(uint tflags =0 ; -if (XFS_IS_REALTIME_MOUNT (mp)) +if (XFS_IS_REALTIME_MOUNT (mp)) {xfs_rt_alloc_min (ip, count); +xfs_rt_fallback (ip, mp); + } rt =XFS_IS_REALTIME_INODE (ip); extsz =xfs_get_extsz_hint (ip);
@@ -986,8 +988,10 @@ xfs_file_iomap_begin(if (XFS_FORCED_SHUTDOWN (mp))return -EIO ; -if (XFS_IS_REALTIME_MOUNT (mp)) +if (XFS_IS_REALTIME_MOUNT (mp)) {xfs_rt_alloc_min (ip, length); +xfs_rt_fallback (ip, mp); + }if (((flags & (IOMAP_WRITE |IOMAP_DIRECT )) ==IOMAP_WRITE ) && !IS_DAX (inode) && !xfs_get_extsz_hint (ip)) {
fs/xfs/xfs_mount.c +26 -1
@@ -509,7 +509,6 @@ xfs_set_low_space_thresholds( } } -/* * Set whether we're using inode alignment. */
@@ -1396,3 +1395,29 @@ xfs_dev_is_read_only( }return 0 ; } + +/* +* precalculate minimum of data blocks required, if we fall +* below this value, we will fallback to the real-time device. +* +* m_rt_fallback_pct can only be non-zero if a real-time device +* is configured. +*/ +void +xfs_set_rt_min_fdblocks ( +struct xfs_mount *mp) +{ +if (mp->m_rt_fallback_pct) { +xfs_sb_t *sbp = &mp->m_sb; +xfs_extlen_t lsize; +__uint64_t min_blocks; + + lsize = sbp->sb_logstart ? sbp->sb_logblocks :0 ; + min_blocks = (mp->m_sb.sb_dblocks - lsize) * mp->m_rt_fallback_pct; +do_div (min_blocks,100 ); +/* Pre-compute minimum data blocks required before +* falling back to RT device for allocations +*/ + mp->m_rt_min_fdblocks = min_blocks; + } +}
fs/xfs/xfs_mount.h +6 -1
@@ -197,7 +197,11 @@ typedef struct xfs_mount {__uint32_t m_generation;bool m_fail_unmount; -uint m_rt_alloc_min;/* Min RT allocation */ +uint m_rt_alloc_min;/* Min RT allocation */ +__uint8_t m_rt_fallback_pct;/* Fall back to realtime device if +* data dev above rt_fallback_pct +*/ +__uint64_t m_rt_min_fdblocks;/* Realtime min fdblock threshold */ #ifdef DEBUG /* * DEBUG mode instrumentation to test and/or trigger delayed allocation
@@ -463,4 +467,5 @@ int xfs_zero_extent(struct xfs_inode *ip, xfs_fsblock_t start_fsb,struct xfs_error_cfg *xfs_error_get_cfg (struct xfs_mount *mp,int error_class,int error); +void xfs_set_rt_min_fdblocks (struct xfs_mount *mp);#endif /* __XFS_MOUNT_H__ */
fs/xfs/xfs_rtalloc.c +14 -0
@@ -1310,3 +1310,17 @@ void xfs_rt_alloc_min( } } } + +void xfs_rt_fallback ( +struct xfs_inode *ip, +struct xfs_mount *mp) +{ +if (!XFS_IS_REALTIME_INODE (ip)) { +__uint64_t free; + free =percpu_counter_sum (&mp->m_fdblocks) - + mp->m_alloc_set_aside; +if (free < mp->m_rt_min_fdblocks) { + ip->i_d.di_flags |=XFS_DIFLAG_REALTIME ; + } + } +}
fs/xfs/xfs_rtalloc.h +2 -1
@@ -137,7 +137,7 @@ int xfs_rtalloc_query_all(struct xfs_trans *tp,xfs_rtalloc_query_range_fn fn, void *priv);void xfs_rt_alloc_min (struct xfs_inode *ip,xfs_off_t len); - +void xfs_rt_fallback (struct xfs_inode *ip,struct xfs_mount *mp);#else # define xfs_rtallocate_extent (t,b,min,max,l,f,p,rb) (ENOSYS)
@@ -159,6 +159,7 @@ xfs_rtmount_init(# define xfs_rtmount_inodes (m) (((mp)->m_sb.sb_rblocks == 0)? 0 : (ENOSYS))# define xfs_rtunmount_inodes (m)# define xfs_rt_alloc_min (i,l) (ENOSYS) +# define xfs_rt_fallback (i,m) (ENOSYS)#endif /* CONFIG_XFS_RT */ #endif /* __XFS_RTALLOC_H__ */
fs/xfs/xfs_sysfs.c +39 -0
@@ -165,6 +165,44 @@ rt_alloc_min_show(return snprintf (buf,PAGE_SIZE ,"%d\n" , mp->m_rt_alloc_min); }XFS_SYSFS_ATTR_RW (rt_alloc_min); + +STATIC ssize_t +rt_fallback_pct_store ( +struct kobject *kobject, +const char *buf, +size_t count) +{ +struct xfs_mount *mp =to_mp (kobject); +int ret; +int val; + + ret =kstrtoint (buf,0 , &val); +if (ret) +return ret; + +/* Only valid if using a real-time device */ +if (XFS_IS_REALTIME_MOUNT (mp) && ((val >0 ) && (val <=100 ))) { + mp->m_rt_fallback_pct = val; +xfs_set_rt_min_fdblocks (mp); + }else if (val <=0 ) { + mp->m_rt_fallback_pct =0 ; + mp->m_rt_min_fdblocks =0 ; + }else +return -EINVAL ; + +return count; +} + +STATIC ssize_t +rt_fallback_pct_show ( +struct kobject *kobject, +char *buf) +{ +struct xfs_mount *mp =to_mp (kobject); + +return snprintf (buf,PAGE_SIZE ,"%d\n" , mp->m_rt_fallback_pct); +} +XFS_SYSFS_ATTR_RW (rt_fallback_pct);#endif /* CONFIG_XFS_RT */ static struct attribute *xfs_mp_attrs[] = {
@@ -173,6 +211,7 @@ static struct attribute *xfs_mp_attrs[] = {#endif #ifdef CONFIG_XFS_RT ATTR_LIST (rt_alloc_min), +ATTR_LIST (rt_fallback_pct), #endif NULL, };
Signature · 1 line
2.9.5
Re: [PATCH v4 3/3] xfs: Add realtime fallback if data device full
Quoted 28 lines · Richard Wareing
- Adds tunable option to fallback to realtime device if configured when data device is full.
- Useful for realtime device users to help prevent ENOSPC errors when selectively storing some files (e.g. small files) on data device, while others are stored on realtime block device.
- Set via the "rt_fallback_pct" sysfs value which is available if the kernel is compiled withCONFIG_XFS_RT.Signed-off-by Richard Wareing <[email protected]>---
Changes since v3:
* None, new patch to patch setfs/xfs/xfs_bmap_util.c | 4 +++- fs/xfs/xfs_fsops.c | 4 ++++ fs/xfs/xfs_iomap.c | 8 ++++++-- fs/xfs/xfs_mount.c | 27 ++++++++++++++++++++++++++- fs/xfs/xfs_mount.h | 7 ++++++- fs/xfs/xfs_rtalloc.c | 14 ++++++++++++++ fs/xfs/xfs_rtalloc.h | 3 ++- fs/xfs/xfs_sysfs.c | 39 +++++++++++++++++++++++++++++++++++++++ 8 files changed, 100 insertions(+), 6 deletions(-)fs/xfs/xfs_bmap_util.c +0
-0
fs/xfs/xfs_bmap_util.c xfs_alloc_file_space( · line 1032
Quoted 8 lines
fs/xfs/xfs_bmap_util.c +3
-1@@ -1026,8 +1026,10 @@ xfs_alloc_file_space(if (len <=0 )return -EINVAL ; -if (XFS_IS_REALTIME_MOUNT (mp)) +if (XFS_IS_REALTIME_MOUNT (mp)) {xfs_rt_alloc_min (ip, len); +xfs_rt_fallback (ip, mp); + }
This should really go inside xfs_inode_select_target() as space availability is just another selection criteria....
fs/xfs/xfs_fsops.c xfs_growfs_data_private( · line 615
Quoted 13 lines
fs/xfs/xfs_bmap_util.c +0
-0rt =XFS_IS_REALTIME_INODE (ip); extsz =xfs_get_extsz_hint (ip);fs/xfs/xfs_fsops.c +3
-0@@ -610,6 +610,10 @@ xfs_growfs_data_private(xfs_set_low_space_thresholds (mp); mp->m_alloc_set_aside =xfs_alloc_set_aside (mp); +if (XFS_IS_REALTIME_MOUNT (mp)) { +xfs_set_rt_min_fdblocks (mp); + }
Normal way to do this is something like:
mp->m_rt_min_fdblocks = xfs_calc_min_free_rtblocks(mp);
and check XFS_IS_REALTIME_MOUNT() inside that function.
And now, reading on, I find I've completely misunderstood what those variable and function names mean, so they need renaming to be clearer.
Quoted 6 lines
fs/xfs/xfs_fsops.c +6
-0+ +/* +* precalculate minimum of data blocks required, if we fall +* below this value, we will fallback to the real-time device. +* +* m_rt_fallback_pct can only be non-zero if a real-time device
fs/xfs/xfs_fsops.c xfs_growfs_data_private(
Quoted 10 lines
fs/xfs/xfs_fsops.c +10
-0+* is configured. +*/ +void +xfs_set_rt_min_fdblocks ( +struct xfs_mount *mp) +{ +if (mp->m_rt_fallback_pct) { +xfs_sb_t *sbp = &mp->m_sb; +xfs_extlen_t lsize; +__uint64_t min_blocks;
Stray whitespace. If you use vim, add this macro to your .vimrc so that it highlights stray whitespace for you:
" highlight whitespace damage
highlight RedundantSpaces ctermbg=red guibg=red
match RedundantSpaces /\s\+$\| \+\ze\t/
fs/xfs/xfs_fsops.c
Quoted 4 lines
fs/xfs/xfs_fsops.c +4
-0+ + lsize = sbp->sb_logstart ? sbp->sb_logblocks :0 ; + min_blocks = (mp->m_sb.sb_dblocks - lsize) * mp->m_rt_fallback_pct; +do_div (min_blocks,100 );
Why bother with the log size?
fs/xfs/xfs_fsops.c
Quoted 3 lines
fs/xfs/xfs_fsops.c +3
-0+/* Pre-compute minimum data blocks required before +* falling back to RT device for allocations +*/
Comment format.
fs/xfs/xfs_fsops.c
Quoted 1 line
fs/xfs/xfs_fsops.c +1
-0+ mp->m_rt_min_fdblocks = min_blocks;
Hmmm - I wonder if it would be better to tie this into the existing data device low space threshold code?
fs/xfs/xfs_fsops.c
Quoted 1 line
fs/xfs/xfs_fsops.c +1
-0+ }
Also, indenting.
if (!XFS_IS_REALTIME_MOUNT(mp)) return 0; if (!mp->m_rt_fallback_pct) return 0; ....
fs/xfs/xfs_mount.h typedef struct xfs_mount { · line 201
Quoted 12 lines
fs/xfs/xfs_fsops.c +1
-0+}fs/xfs/xfs_mount.h +2
-1@@ -197,7 +197,11 @@ typedef struct xfs_mount {__uint32_t m_generation;bool m_fail_unmount; -uint m_rt_alloc_min;/* Min RT allocation */ +uint m_rt_alloc_min;/* Min RT allocation */ +__uint8_t m_rt_fallback_pct;/* Fall back to realtime device if
uint32_t is fine. We're moving away from the __[u]int types, so we shouldn't really add any new ones.
fs/xfs/xfs_mount.h typedef struct xfs_mount { · line 204
Quoted 3 lines
fs/xfs/xfs_mount.h +3
-0+void xfs_rt_fallback ( +struct xfs_inode *ip, +struct xfs_mount *mp)
Mount first, then inode.
fs/xfs/xfs_mount.h typedef struct xfs_mount {
Quoted 10 lines
fs/xfs/xfs_mount.h +10
-0+{ +if (!XFS_IS_REALTIME_INODE (ip)) { +__uint64_t free; + free =percpu_counter_sum (&mp->m_fdblocks) - + mp->m_alloc_set_aside; +if (free < mp->m_rt_min_fdblocks) { + ip->i_d.di_flags |=XFS_DIFLAG_REALTIME ; + } + } +}
Indenting, but should really move to the target selection function.
Quoted 13 lines
fs/xfs/xfs_mount.h +13
-0+STATIC ssize_t +rt_fallback_pct_store ( +struct kobject *kobject, +const char *buf, +size_t count) +{ +struct xfs_mount *mp =to_mp (kobject); +int ret; +int val; + + ret =kstrtoint (buf,0 , &val); +if (ret) +return ret;
fs/xfs/xfs_mount.h
Quoted 10 lines
fs/xfs/xfs_mount.h +10
-0+ +/* Only valid if using a real-time device */ +if (XFS_IS_REALTIME_MOUNT (mp) && ((val >0 ) && (val <=100 ))) { + mp->m_rt_fallback_pct = val; +xfs_set_rt_min_fdblocks (mp); + }else if (val <=0 ) { + mp->m_rt_fallback_pct =0 ; + mp->m_rt_min_fdblocks =0 ; + }else +return -EINVAL ;
Same issue as last patch.
Also, just set then threshold percentage, and if there is no error, then call xfs_set_rt_min_fdblocks() rather than zeroing it directly.
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH v4 3/3] xfs: Add realtime fallback if data device full
Quoted 32 lines · Dave Chinner
Quoted 31 lines · Richard Wareing
- Adds tunable option to fallback to realtime device if configured when data device is full.
- Useful for realtime device users to help prevent ENOSPC errors when selectively storing some files (e.g. small files) on data device, while others are stored on realtime block device.
- Set via the "rt_fallback_pct" sysfs value which is available if the kernel is compiled withCONFIG_XFS_RT.Signed-off-by Richard Wareing <[email protected]>---
Changes since v3:
* None, new patch to patch setfs/xfs/xfs_bmap_util.c | 4 +++- fs/xfs/xfs_fsops.c | 4 ++++ fs/xfs/xfs_iomap.c | 8 ++++++-- fs/xfs/xfs_mount.c | 27 ++++++++++++++++++++++++++- fs/xfs/xfs_mount.h | 7 ++++++- fs/xfs/xfs_rtalloc.c | 14 ++++++++++++++ fs/xfs/xfs_rtalloc.h | 3 ++- fs/xfs/xfs_sysfs.c | 39 +++++++++++++++++++++++++++++++++++++++ 8 files changed, 100 insertions(+), 6 deletions(-)fs/xfs/xfs_bmap_util.c +0
-0@@ -1026,8 +1026,10 @@ xfs_alloc_file_space(if (len <=0 )return -EINVAL ;
Quoted 9 lines
fs/xfs/xfs_bmap_util.c xfs_alloc_file_space( · line 1032
Quoted 5 lines
fs/xfs/xfs_bmap_util.c +3
-1-if (XFS_IS_REALTIME_MOUNT (mp)) +if (XFS_IS_REALTIME_MOUNT (mp)) {xfs_rt_alloc_min (ip, len); +xfs_rt_fallback (ip, mp); + }This should really go inside
xfs_inode_select_target()as space availability is just another selection criteria....
Re-worked this. I like this idea better as well.
Quoted 16 lines
fs/xfs/xfs_fsops.c xfs_growfs_data_private( · line 615
Quoted 13 lines
fs/xfs/xfs_bmap_util.c +0
-0rt =XFS_IS_REALTIME_INODE (ip); extsz =xfs_get_extsz_hint (ip);fs/xfs/xfs_fsops.c +3
-0@@ -610,6 +610,10 @@ xfs_growfs_data_private(xfs_set_low_space_thresholds (mp); mp->m_alloc_set_aside =xfs_alloc_set_aside (mp); +if (XFS_IS_REALTIME_MOUNT (mp)) { +xfs_set_rt_min_fdblocks (mp); + }Normal way to do this is something like:
Quoted 7 lines
mp->m_rt_min_fdblocks = xfs_calc_min_free_rtblocks(mp);and check
XFS_IS_REALTIME_MOUNT()inside that function.And now, reading on, I find I've completely misunderstood what those variable and function names mean, so they need renaming to be clearer.
Fixed in new version, in my defense I was modeling this after xfs_set_low_space_thresholds, but looks like xfs_alloc_set_aside as you point out is a better pattern to follow (since mine is a trivial assignment).
Quoted 21 lines
Quoted 7 lines
fs/xfs/xfs_fsops.c +6
-0+ +/* +* precalculate minimum of data blocks required, if we fall +* below this value, we will fallback to the real-time device. +* +* m_rt_fallback_pct can only be non-zero if a real-time device fs/xfs/xfs_fsops.c xfs_growfs_data_private(
Quoted 10 lines
fs/xfs/xfs_fsops.c +10
-0+* is configured. +*/ +void +xfs_set_rt_min_fdblocks ( +struct xfs_mount *mp) +{ +if (mp->m_rt_fallback_pct) { +xfs_sb_t *sbp = &mp->m_sb; +xfs_extlen_t lsize; +__uint64_t min_blocks;Stray whitespace. If you use vim, add this macro to your .vimrc so that it highlights stray whitespace for you:
Quoted 4 lines
" highlight whitespace damage
highlight RedundantSpaces ctermbg=red guibg=red
match RedundantSpaces /\s\+$\| \+\ze\t/
Thanks for this, added to my vimrc!
Quoted 7 lines
fs/xfs/xfs_fsops.c
Quoted 4 lines
fs/xfs/xfs_fsops.c +4
-0+ + lsize = sbp->sb_logstart ? sbp->sb_logblocks :0 ; + min_blocks = (mp->m_sb.sb_dblocks - lsize) * mp->m_rt_fallback_pct; +do_div (min_blocks,100 );Why bother with the log size?
My thinking was to subtract out blocks used by the log, but upon further reflection this isn't correct since the log blocks are already decremented out of the free block count. I've fixed this up.
Quoted 11 lines
fs/xfs/xfs_fsops.c
Quoted 3 lines
fs/xfs/xfs_fsops.c +3
-0+/* Pre-compute minimum data blocks required before +* falling back to RT device for allocations +*/ Comment format.
fs/xfs/xfs_fsops.c
Quoted 1 line
fs/xfs/xfs_fsops.c +1
-0+ mp->m_rt_min_fdblocks = min_blocks;Hmmm - I wonder if it would be better to tie this into the existing data device low space threshold code?
Not sure what you mean here?
Quoted 20 lines
fs/xfs/xfs_fsops.c
Quoted 1 line
fs/xfs/xfs_fsops.c +1
-0+ }Also, indenting.
if (!XFS_IS_REALTIME_MOUNT(mp)) return 0; if (!mp->m_rt_fallback_pct) return 0; ....Quoted 8 lines
fs/xfs/xfs_fsops.c +1
-0+}fs/xfs/xfs_mount.h +0
-0@@ -197,7 +197,11 @@ typedef struct xfs_mount {__uint32_t m_generation;
Quoted 8 lines
fs/xfs/xfs_mount.h typedef struct xfs_mount { · line 201
Quoted 4 lines
fs/xfs/xfs_mount.h +2
-1bool m_fail_unmount; -uint m_rt_alloc_min;/* Min RT allocation */ +uint m_rt_alloc_min;/* Min RT allocation */ +__uint8_t m_rt_fallback_pct;/* Fall back to realtime device if
uint32_tis fine. We're moving away from the __[u]int types, so we shouldn't really add any new ones.
Fixed.
Quoted 6 lines
fs/xfs/xfs_mount.h typedef struct xfs_mount { · line 204
Quoted 3 lines
fs/xfs/xfs_mount.h +3
-0+void xfs_rt_fallback ( +struct xfs_inode *ip, +struct xfs_mount *mp)Mount first, then inode.
Just for my own knowledge, is this a convention?
Quoted 4 lines
Quoted 4 lines
fs/xfs/xfs_mount.h +4
-0+{ +if (!XFS_IS_REALTIME_INODE (ip)) { +__uint64_t free; + free = percpu_counter_sum(&mp->m_fdblocks) -
Quoted 9 lines
fs/xfs/xfs_mount.h
Quoted 6 lines
fs/xfs/xfs_mount.h +6
-0+ mp->m_alloc_set_aside; +if (free < mp->m_rt_min_fdblocks) { + ip->i_d.di_flags |=XFS_DIFLAG_REALTIME ; + } + } +}Indenting, but should really move to the target selection function.
Done. I added a target function and two functions to contain the policy code specific to each threshold.
Quoted 26 lines
Quoted 13 lines
fs/xfs/xfs_mount.h +13
-0+STATIC ssize_t +rt_fallback_pct_store ( +struct kobject *kobject, +const char *buf, +size_t count) +{ +struct xfs_mount *mp =to_mp (kobject); +int ret; +int val; + + ret =kstrtoint (buf,0 , &val); +if (ret) +return ret;fs/xfs/xfs_mount.h
Quoted 10 lines
fs/xfs/xfs_mount.h +10
-0+ +/* Only valid if using a real-time device */ +if (XFS_IS_REALTIME_MOUNT (mp) && ((val >0 ) && (val <=100 ))) { + mp->m_rt_fallback_pct = val; +xfs_set_rt_min_fdblocks (mp); + }else if (val <=0 ) { + mp->m_rt_fallback_pct =0 ; + mp->m_rt_min_fdblocks =0 ; + }else +return -EINVAL ;Same issue as last patch.
Quoted 3 lines
Also, just set then threshold percentage, and if there is no error, then call
xfs_set_rt_min_fdblocks()rather than zeroing it directly.
Fixed.
Quoted 6 lines
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH v4 3/3] xfs: Add realtime fallback if data device full
Quoted 8 lines · Richard Wareing
Quoted 7 lines · Dave Chinner
fs/xfs/xfs_mount.c xfs_dev_is_read_only( · line 1420
Quoted 3 lines · Richard Wareing
fs/xfs/xfs_mount.c +3
-0+/* Pre-compute minimum data blocks required before +* falling back to RT device for allocations +*/ Comment format.
Quoted 7 lines
Quoted 5 lines
fs/xfs/xfs_mount.c xfs_dev_is_read_only( · line 1421
Quoted 1 line
fs/xfs/xfs_mount.c +1
-0+ mp->m_rt_min_fdblocks = min_blocks;Hmmm - I wonder if it would be better to tie this into the existing data device low space threshold code?
Not sure what you mean here?
The lowspace threshold code is just an array of thresholds at which we do trigger different behaviour. e.g. we trim maximum speculative delalloc space based one where free space falls in that table. If we add another entry in the table for the "switch to RT device at this low space" threshold, we don't need a specific variable in the struct xfs_mount....
Quoted 9 lines
Quoted 7 lines
fs/xfs/xfs_rtalloc.c void xfs_rt_alloc_min( · line 1316
Quoted 4 lines
fs/xfs/xfs_rtalloc.c +3
-0+void xfs_rt_fallback ( +struct xfs_inode *ip, +struct xfs_mount *mp)Mount first, then inode.
Just for my own knowledge, is this a convention?
Convention. I also missed that the return type goes on a separate line. i.e.
void
xfs_rt_fallback(
struct xfs_mount *mp,
struct xfs_inode *ip)
It's different to the standard linux convention for a couple of reasons. Firstly, it's the historic format inherited from Irix but we kept it because it makes using grep to find a function declaration really easy (i.e. "^xfs_rt_fallback" finds the function declaration instead of all the callers) and there are functions with lots of parameters and the "run them all together in as few lines as possible" is hard on the eyes and requires reformatting of the entire function definition when one changes.
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]