xfs: outstanding patches for 3.4 merge window
cover xfs: outstanding patches for 3.4 merge window
The first 5 patches have been previously posted and reviewed, but not yet comitted to the XFS repo.
The last three patches are new. The first two fix reported bugs and are stable kernel backport candidates, and the last is an update to the attribute event tracing code to add some insight into the attribute modification paths. This one might be 3.5 material, but it can't hurt to try.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 1/8 xfs: Fix open flag handling in open_by_handle code
From: Dave Chinner <[email protected]>
Sparse identified some unsafe handling of open flags in the xfs open by handle ioctl code. Update the code to use the correct access macros to ensure that we handle the open flags correctly.
--- fs/xfs/xfs_ioctl.c | 14 +++++--------- 1 files changed, 5 insertions(+), 9 deletions(-)
fs/xfs/xfs_ioctl.c +5 -9
@@ -209,6 +209,7 @@ xfs_open_by_handle(struct file *filp;struct inode *inode;struct dentry *dentry; +fmode_t fmode;if (!capable (CAP_SYS_ADMIN ))return -XFS_ERROR (EPERM );
@@ -228,26 +229,21 @@ xfs_open_by_handle( hreq->oflags |=O_LARGEFILE ;#endif -/* Put open permission in namei format. */ permflag = hreq->oflags; -if ((permflag+1 ) &O_ACCMODE ) - permflag++; -if (permflag &O_TRUNC ) - permflag |=2 ; - + fmode =OPEN_FMODE (permflag);if ((!(permflag &O_APPEND ) || (permflag &O_TRUNC )) && - (permflag &FMODE_WRITE ) &&IS_APPEND (inode)) { + (fmode &FMODE_WRITE ) &&IS_APPEND (inode)) { error = -XFS_ERROR (EPERM ); goto out_dput; } -if ((permflag &FMODE_WRITE ) &&IS_IMMUTABLE (inode)) { +if ((fmode &FMODE_WRITE ) &&IS_IMMUTABLE (inode)) { error = -XFS_ERROR (EACCES ); goto out_dput; }/* Can't write directories. */ -if (S_ISDIR (inode->i_mode) && (permflag &FMODE_WRITE )) { +if (S_ISDIR (inode->i_mode) && (fmode &FMODE_WRITE )) { error = -XFS_ERROR (EISDIR ); goto out_dput; }
Signature · 6 lines
1.7.9 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 2/8 xfs: introduce an allocation workqueue
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
We currently have significant issues with the amount of stack that allocation in XFS uses, especially in the writeback path. We can easily consume 4k of stack between mapping the page, manipulating the bmap btree and allocating blocks from the free list. Not to mention btree block readahead and other functionality that issues IO in the allocation path.
As a result, we can no longer fit allocation in the writeback path in the stack space provided on x86_64. To alleviate this problem, introduce an allocation workqueue and move all allocations to a seperate context. This can be easily added as an interposing layer into xfs_alloc_vextent(), which takes a single argument structure and does not return until the allocation is complete or has failed.
To do this, add a work structure and a completion to the allocation args structure. This allows xfs_alloc_vextent to queue the args onto the workqueue and wait for it to be completed by the worker. This can be done completely transparently to the caller.
The worker function needs to ensure that it sets and clears the PF_TRANS flag appropriately as it is being run in an active transaction context. Work can also be queued in a memory reclaim context, so a rescuer is needed for the workqueue.
--- fs/xfs/xfs_alloc.c | 34 +++++++++++++++++++++++++++++++++- fs/xfs/xfs_alloc.h | 5 +++++ fs/xfs/xfs_super.c | 16 ++++++++++++++++ 3 files changed, 54 insertions(+), 1 deletions(-)
fs/xfs/xfs_alloc.c +33 -1
@@ -35,6 +35,7 @@#include "xfs_error.h" #include "xfs_trace.h" +struct workqueue_struct *xfs_alloc_wq;#define XFS_ABSDIFF (a,b) (((a) <= (b)) ? ((b) - (a)) : ((a) - (b)))
@@ -2207,7 +2208,7 @@ xfs_alloc_read_agf(* group or loop over the allocation groups to find the result. */ int /* error */ -xfs_alloc_vextent ( +__xfs_alloc_vextent (xfs_alloc_arg_t *args)/* allocation argument structure */ {xfs_agblock_t agsize;/* allocation group size */
@@ -2417,6 +2418,37 @@ error0:return error; } +static void +xfs_alloc_vextent_worker ( +struct work_struct *work) +{ +struct xfs_alloc_arg *args =container_of (work, + struct xfs_alloc_arg, work); +unsigned long pflags; + +/* we are in a transaction context here */ +current_set_flags_nested (&pflags,PF_FSTRANS ); + + args->result =__xfs_alloc_vextent (args); +complete (args->done); + +current_restore_flags_nested (&pflags,PF_FSTRANS ); +} + + +int /* error */ +xfs_alloc_vextent ( +xfs_alloc_arg_t *args)/* allocation argument structure */ +{ +DECLARE_COMPLETION_ONSTACK (done); + + args->done = &done; +INIT_WORK (&args->work, xfs_alloc_vextent_worker); +queue_work (xfs_alloc_wq, &args->work); +wait_for_completion (&done); +return args->result; +} +/* * Free an extent. * Just break up the extent address and hand off to xfs_free_ag_extent
fs/xfs/xfs_alloc.h +5 -0
@@ -25,6 +25,8 @@ struct xfs_perag;struct xfs_trans ;struct xfs_busy_extent ; +extern struct workqueue_struct *xfs_alloc_wq; +/* * Freespace allocation types. Argument to xfs_alloc_[v]extent. */
@@ -119,6 +121,9 @@ typedef struct xfs_alloc_arg {char isfl;/* set if is freelist blocks - !acctg */ char userdata;/* set if this is user data */ xfs_fsblock_t firstblock;/* io first block allocated */ +struct completion *done; +struct work_struct work; +int result; } xfs_alloc_arg_t;/*
fs/xfs/xfs_super.c +16 -0
@@ -1606,12 +1606,28 @@ xfs_init_workqueues(void) xfs_syncd_wq =alloc_workqueue ("xfssyncd" ,WQ_NON_REENTRANT ,0 );if (!xfs_syncd_wq)return -ENOMEM ; + +/* +* The allocation workqueue can be used in memory reclaim situations +* (writepage path), and parallelism is only limited by the number of +* AGs in all the filesystems mounted. Hence use the default large +* max_active value for this workqueue. +*/ + xfs_alloc_wq =alloc_workqueue ("xfsalloc" ,WQ_MEM_RECLAIM ,0 ); +if (!xfs_alloc_wq) + goto out_destroy_syncd; +return 0 ; + +out_destroy_syncd: +destroy_workqueue (xfs_syncd_wq); +return -ENOMEM ; } STATIC voidxfs_destroy_workqueues (void ) { +destroy_workqueue (xfs_alloc_wq);destroy_workqueue (xfs_syncd_wq); }
Signature · 6 lines
1.7.9 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 3/8 xfs: initialise xfssync work before running quotachecks
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
Because the mount process can run a quotacheck and consume lots of inodes, we need to be able to run periodic inode reclaim during the mount process. This will prevent running the system out of memory during quota checks.
This essentially reverts 2bcf6e97, but that is safe to do now that the quota sync code that was causing problems during long quotacheck executions is now gone.
--- fs/xfs/xfs_super.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-)
fs/xfs/xfs_super.c +9 -8
@@ -1346,31 +1346,33 @@ xfs_fs_fill_super( sb->s_time_gran =1 ;set_posix_acl_flag (sb); - error =xfs_mountfs (mp); + error =xfs_syncd_init (mp);if (error) goto out_filestream_unmount; - error =xfs_syncd_init (mp); + error =xfs_mountfs (mp);if (error) - goto out_unmount; + goto out_syncd_stop; root =igrab (VFS_I (mp->m_rootip));if (!root) { error =ENOENT ; - goto out_syncd_stop; + goto out_unmount; }if (is_bad_inode (root)) { error =EINVAL ; - goto out_syncd_stop; + goto out_unmount; } sb->s_root =d_make_root (root);if (!sb->s_root) { error =ENOMEM ; - goto out_syncd_stop; + goto out_unmount; }return 0 ; + out_syncd_stop: +xfs_syncd_stop (mp); out_filestream_unmount:xfs_filestream_unmount (mp); out_free_sb:
@@ -1387,8 +1389,6 @@ out_destroy_workqueues: out:return -error; - out_syncd_stop: -xfs_syncd_stop (mp); out_unmount:/* * Blow away any referenced inode in the filestreams cache.
@@ -1400,6 +1400,7 @@ out_destroy_workqueues:xfs_flush_buftarg (mp->m_ddev_targp,1 );xfs_unmountfs (mp); +xfs_syncd_stop (mp); goto out_free_sb; }
Signature · 6 lines
1.7.9 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/8] xfs: initialise xfssync work before running quotachecks
Quoted 10 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Because the mount process can run a quotacheck and consume lots of inodes, we need to be able to run periodic inode reclaim during the mount process. This will prevent running the system out of memory during quota checks.
This essentially reverts
2bcf6e97, but that is safe to do now that the quota sync code that was causing problems during long quotacheck executions is now gone.
Dave, I've held off on #s 3 and 4 because they appear to be racy. Being able to run inode reclaim during quota check seems like a good idea though.
-Ben
Quoted 74 lines
Signed-off-by Dave Chinner <[email protected]>Reviewed-by Christoph Hellwig <[email protected]>--- fs/xfs/xfs_super.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-)fs/xfs/xfs_super.c +9
-8@@ -1346,31 +1346,33 @@ xfs_fs_fill_super( sb->s_time_gran =1 ;set_posix_acl_flag (sb); - error =xfs_mountfs (mp); + error =xfs_syncd_init (mp);if (error) goto out_filestream_unmount; - error =xfs_syncd_init (mp); + error =xfs_mountfs (mp);if (error) - goto out_unmount; + goto out_syncd_stop; root =igrab (VFS_I (mp->m_rootip));if (!root) { error =ENOENT ; - goto out_syncd_stop; + goto out_unmount; }if (is_bad_inode (root)) { error =EINVAL ; - goto out_syncd_stop; + goto out_unmount; } sb->s_root =d_make_root (root);if (!sb->s_root) { error =ENOMEM ; - goto out_syncd_stop; + goto out_unmount; }return 0 ; + out_syncd_stop: +xfs_syncd_stop (mp); out_filestream_unmount:xfs_filestream_unmount (mp); out_free_sb:@@ -1387,8 +1389,6 @@ out_destroy_workqueues: out:return -error; - out_syncd_stop: -xfs_syncd_stop (mp); out_unmount:/* * Blow away any referenced inode in the filestreams cache. @@ -1400,6 +1400,7 @@ out_destroy_workqueues:xfs_flush_buftarg (mp->m_ddev_targp,1 );xfs_unmountfs (mp); +xfs_syncd_stop (mp); goto out_free_sb; }Signature · 6 lines
1.7.9 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/8] xfs: initialise xfssync work before running quotachecks
Quoted 8 lines · Ben Myers
Quoted 7 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Because the mount process can run a quotacheck and consume lots of inodes, we need to be able to run periodic inode reclaim during the mount process. This will prevent running the system out of memory during quota checks.
Quoted 5 lines
Quoted 3 lines
This essentially reverts
2bcf6e97, but that is safe to do now that the quota sync code that was causing problems during long quotacheck executions is now gone.Dave, I've held off on #s 3 and 4 because they appear to be racy. Being
What race?
Cheers,
Dave
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/8] xfs: initialise xfssync work before running quotachecks
Quoted 13 lines · Dave Chinner
Quoted 12 lines · Ben Myers
Quoted 10 lines · Dave Chinner
From: Dave Chinner<[email protected]>
Because the mount process can run a quotacheck and consume lots of inodes, we need to be able to run periodic inode reclaim during the mount process. This will prevent running the system out of memory during quota checks.
This essentially reverts
2bcf6e97, but that is safe to do now that the quota sync code that was causing problems during long quotacheck executions is now gone.
Quoted 7 lines
Quoted 1 line
Dave, I've held off on #s 3 and 4 because they appear to be racy. Being
What race?
Cheers,
Dave
2 of the sync workers use iterators
xfs_inode_ag_iterator()
xfs_perag_get()
radix_tree_lookup(&mp->m_perag_tree, agno)
The race I was worried about was in xfs_mount() to initialize the mp->m_perag_lock, and the radix tree initialization:
INIT_RADIX_TREE(&mp->m_perag_tree, GFP_ATOMIC)).
There is a lock and 2 or 3 unbuffered I/O are performed in xfs_mountfs() before the mp->m_perag_tree is initialized.
I was also looking at the xfs_perag_t being allocated in mountfs() and being deallocated in umountfs(), but it turns out that is not important, xfs_perag_get() will return NULL if these have not been allocated yet or have been removed for the required ag.
Mark Tinguely.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/8] xfs: initialise xfssync work before running quotachecks
Quoted 28 lines · Mark Tinguely
Quoted 13 lines · Dave Chinner
Quoted 12 lines · Ben Myers
Quoted 10 lines · Dave Chinner
From: Dave Chinner<[email protected]>
Because the mount process can run a quotacheck and consume lots of inodes, we need to be able to run periodic inode reclaim during the mount process. This will prevent running the system out of memory during quota checks.
This essentially reverts
2bcf6e97, but that is safe to do now that the quota sync code that was causing problems during long quotacheck executions is now gone.Quoted 7 lines
Quoted 1 line
Dave, I've held off on #s 3 and 4 because they appear to be racy. Being
What race?
Cheers,
Dave
2 of the sync workers use iterators
xfs_inode_ag_iterator()
xfs_perag_get()
radix_tree_lookup(&mp->m_perag_tree, agno)
Quoted 6 lines
The race I was worried about was in
xfs_mount()to initialize themp->m_perag_lock, and the radix tree initialization:
INIT_RADIX_TREE(&mp->m_perag_tree, GFP_ATOMIC)).There is a lock and 2 or 3 unbuffered I/O are performed in
xfs_mountfs()before themp->m_perag_treeis initialized.
Yes they are uncached IOs so do not utilise the cache that requires the mp->m_perag_tree to be initialised.
Quoted 4 lines
I was also looking at the
xfs_perag_tbeing allocated inmountfs()and being deallocated inumountfs(), but it turns out that is not important,xfs_perag_get()will return NULL if these have not been allocated yet or have been removed for the required ag.
Correct. The other side of that is that if xfssyncd is doing some form of inode cache iteration, it will simply not find any perag structures to scani and hence won't cause problems. Same with the reclaim worker.
Were there any other issues?
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/8] xfs: initialise xfssync work before running quotachecks
Quoted 29 lines · Dave Chinner
Quoted 28 lines · Mark Tinguely
Quoted 28 lines · Dave Chinner
Quoted 13 lines
Quoted 13 lines · Ben Myers
Quoted 12 lines
Quoted 12 lines · Dave Chinner
Quoted 10 lines
Quoted 10 lines
From: Dave Chinner<[email protected]>
Because the mount process can run a quotacheck and consume lots of inodes, we need to be able to run periodic inode reclaim during the mount process. This will prevent running the system out of memory during quota checks.
This essentially reverts
2bcf6e97, but that is safe to do now that the quota sync code that was causing problems during long quotacheck executions is now gone.Quoted 7 lines
Quoted 7 lines
Quoted 1 line
Quoted 1 line
Dave, I've held off on #s 3 and 4 because they appear to be racy. Being
What race?
Cheers,
Dave
2 of the sync workers use iterators
xfs_inode_ag_iterator()xfs_perag_get() radix_tree_lookup(&mp->m_perag_tree, agno)
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
The race I was worried about was in
xfs_mount()to initialize themp->m_perag_lock, and the radix tree initialization:INIT_RADIX_TREE(&mp->m_perag_tree, GFP_ATOMIC)).There is a lock and 2 or 3 unbuffered I/O are performed in
xfs_mountfs()before themp->m_perag_treeis initialized.Yes they are uncached IOs so do not utilise the cache that requires the
mp->m_perag_treeto be initialised.
The point I was trying to make is the sync workers use iterators. The race is to get the mp->m_perag_tree initialized before one of the sync workers tries to do a xfs_perag_get().
I mentioned the lock and the 2 or 3 unbuffered I/O because they are the potential items that can take some time between starting the sync workers and intializing the m_perag_tree radix tree.
Quoted 9 lines
Quoted 4 lines
Quoted 4 lines
I was also looking at the
xfs_perag_tbeing allocated inmountfs()and being deallocated inumountfs(), but it turns out that is not important,xfs_perag_get()will return NULL if these have not been allocated yet or have been removed for the required ag.Correct. The other side of that is that if xfssyncd is doing some form of inode cache iteration, it will simply not find any perag structures to scani and hence won't cause problems. Same with the reclaim worker.
Quoted 6 lines
Were there any other issues?
Cheers,
Dave.
-- Dave Chinner [email protected]
Thanks,
--Mark Tinguely
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/8] xfs: initialise xfssync work before running quotachecks
Quoted 34 lines · Mark Tinguely
Quoted 33 lines · Dave Chinner
Quoted 32 lines · Mark Tinguely
Quoted 32 lines · Dave Chinner
Quoted 13 lines
Quoted 13 lines · Ben Myers
Quoted 12 lines
Quoted 12 lines · Dave Chinner
Quoted 10 lines
Quoted 10 lines
From: Dave Chinner<[email protected]>
Because the mount process can run a quotacheck and consume lots of inodes, we need to be able to run periodic inode reclaim during the mount process. This will prevent running the system out of memory during quota checks.
This essentially reverts
2bcf6e97, but that is safe to do now that the quota sync code that was causing problems during long quotacheck executions is now gone.Quoted 7 lines
Quoted 7 lines
Quoted 1 line
Quoted 1 line
Dave, I've held off on #s 3 and 4 because they appear to be racy. Being
What race?
Cheers,
Dave
2 of the sync workers use iterators
xfs_inode_ag_iterator()xfs_perag_get() radix_tree_lookup(&mp->m_perag_tree, agno)The race I was worried about was in
xfs_mount()to initialize themp->m_perag_lock, and the radix tree initialization:INIT_RADIX_TREE(&mp->m_perag_tree, GFP_ATOMIC)).
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
There is a lock and 2 or 3 unbuffered I/O are performed in
xfs_mountfs()before themp->m_perag_treeis initialized.Yes they are uncached IOs so do not utilise the cache that requires the
mp->m_perag_treeto be initialised.The point I was trying to make is the sync workers use iterators. The race is to get the
mp->m_perag_treeinitialized before one of the sync workers tries to do axfs_perag_get().
Firstly, xfs_sync_worker does not iterate AGs at all anymore - it pushes the log and the AIL, and nothing else. So there is no problems there.
Secondly xfs_flush_worker() is only triggered by ENOSPC, and that can't happen until the filesystem is mounted and real work starts.
Finally, the reclaim worker does iterate the perag tree, but the next patch in the series ensures that is started on demand, not from xfs_syncd_init(). This ensures that iteration does not occur until after the first inode is placed into reclaim, and that must happen after the perag tree is initialised because otherwise we can't read in inodes, let alone put them into a reclaim state....
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/8] xfs: initialise xfssync work before running quotachecks
Hi Dave,
Quoted 40 lines · Dave Chinner
Quoted 39 lines · Mark Tinguely
Quoted 37 lines · Dave Chinner
Quoted 28 lines · Mark Tinguely
Quoted 28 lines · Dave Chinner
Quoted 13 lines
Quoted 13 lines · Ben Myers
Quoted 12 lines
Quoted 12 lines · Dave Chinner
Quoted 10 lines
Quoted 10 lines
From: Dave Chinner<[email protected]>
Because the mount process can run a quotacheck and consume lots of inodes, we need to be able to run periodic inode reclaim during the mount process. This will prevent running the system out of memory during quota checks.
This essentially reverts
2bcf6e97, but that is safe to do now that the quota sync code that was causing problems during long quotacheck executions is now gone.Quoted 7 lines
Quoted 7 lines
Quoted 1 line
Quoted 1 line
Dave, I've held off on #s 3 and 4 because they appear to be racy. Being
What race?
Cheers,
Dave
2 of the sync workers use iterators
xfs_inode_ag_iterator()xfs_perag_get() radix_tree_lookup(&mp->m_perag_tree, agno)Quoted 6 lines
Quoted 6 lines
The race I was worried about was in
xfs_mount()to initialize themp->m_perag_lock, and the radix tree initialization:INIT_RADIX_TREE(&mp->m_perag_tree, GFP_ATOMIC)).There is a lock and 2 or 3 unbuffered I/O are performed in
xfs_mountfs()before themp->m_perag_treeis initialized.Yes they are uncached IOs so do not utilise the cache that requires the
mp->m_perag_treeto be initialised.
Quoted 7 lines
Quoted 3 lines
The point I was trying to make is the sync workers use iterators. The race is to get the
mp->m_perag_treeinitialized before one of the sync workers tries to do axfs_perag_get().Firstly,
xfs_sync_workerdoes not iterate AGs at all anymore - it pushes the log and the AIL, and nothing else. So there is no problems there.
xfs_sync_worker forces the log and pushes the ail, and a sync is queued in xfs_syncd_init before either the log or ail are initialized. A sync should not be queued before the log and ail are initialized regardless of the value of xfs_syncd_centisecs. In the error path of xfs_fs_fill_super a sync could still be running after xfs_unmount is called, so there is a window there where it could dereference m_log which had been set to NULL.
Quoted 2 lines
Secondly
xfs_flush_worker()is only triggered by ENOSPC, and that can't happen until the filesystem is mounted and real work starts.
xfs_flush_worker is triggered by xfs_flush_inodes on ENOSPC in xfs_create and xfs_iomap_write_delay. I agree that in upon startup one would not be able to trigger an ENOSPC event from either of these codepaths until the filesystem has mounted. Further, if we hit any error in fill_super you could not possibly trigger ENOSPC because the root inode had not been allocated yet.
Quoted 1 line
Finally, the reclaim worker does iterate the perag tree,
xfs_reclaim_worker has a similar issue with the perag tree in this patch. It could look at the tree before it has been initialized.
Quoted 5 lines
but the next patch in the series ensures that is started on demand, not from
xfs_syncd_init(). This ensures that iteration does not occur until after the first inode is placed into reclaim, and that must happen after the perag tree is initialised because otherwise we can't read in inodes, let alone put them into a reclaim state....
I suggest 3 and 4 should be combined into one patch.
You've reordered xfs_syncd_stop with respect to xfs_unmount in the error path of xfs_fs_fill_super, but not in the regular unmount path xfs_fs_put_super. I think for consistency they should not be reordered in the error path of xfs_fs_fill_super.
As long as workers can run before xfs_mountfs is run, they need to protect themselves to ensure that the structures they are using are initialized. It looks like xfs_reclaim_worker would do this in the next patch by using MS_ACTIVE, but FWICS xfs_sync_worker still does not protect itself.
-Ben
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/8] xfs: initialise xfssync work before running quotachecks
On Wed, Mar 28, 2012 at 02:40:18PM -0500, Ben Myers wrote:
[ snip repeated arguments about startup races ]
[ insert repeated responses about "fixed in next patch" ]
[ wait for circle to go around again ]
Quoted 1 line
I suggest 3 and 4 should be combined into one patch.
Why didn't you just say that's what you want? I have no problem doing that. I just kept them separate as there are two logical changes to the fix - startup order changes, and MS_ACTIVE guard changes. Do you want me to send a new combined patch?
FWIW, arguing that a patch in a series is unacceptable because introduces a temporary problem that is fixed by another patch in the series is not really helpful, as is saying "it has races" and not explaining them. If you want the temporary problematic state removed from the series, just *say so* - it is easy to do.
Quoted 4 lines
You've reordered
xfs_syncd_stopwith respect toxfs_unmountin the error path ofxfs_fs_fill_super, but not in the regular unmount pathxfs_fs_put_super. I think for consistency they should not be reordered in the error path ofxfs_fs_fill_super.
[ the circle goes around again ]
The second patch makes it consistent in that xfs_syncd_stop() is always called after xfs_unmountfs().
Quoted 4 lines
As long as workers can run before
xfs_mountfsis run, they need to protect themselves to ensure that the structures they are using are initialized. It looks likexfs_reclaim_workerwould do this in the next patch by usingMS_ACTIVE, but FWICSxfs_sync_workerstill does not protect itself.
Go back and read the second patch, and then review what you wrote here. Don't waste my time by making me have to explain it again....
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 1/1 xfs: Ensure inode reclaim can run during quotacheck
Quoted 6 lines · Dave Chinner
On Wed, Mar 28, 2012 at 02:40:18PM -0500, Ben Myers wrote:
[ snip repeated arguments about startup races ]
[ insert repeated responses about "fixed in next patch" ]
Quoted 8 lines
[ wait for circle to go around again ]
Quoted 1 line
I suggest 3 and 4 should be combined into one patch.
Why didn't you just say that's what you want? I have no problem doing that. I just kept them separate as there are two logical changes to the fix - startup order changes, and
MS_ACTIVEguard changes. Do you want me to send a new combined patch?
Here's a new combined patch against current linus+xfsdev TOT.
Cheers,
Dave.
Signature · 33 lines
Dave Chinner [email protected] xfs: Ensure inode reclaim can run during quotacheck From: Dave Chinner <[email protected]> Because the mount process can run a quotacheck and consume lots of inodes, we need to be able to run periodic inode reclaim during the mount process. This will prevent running the system out of memory during quota checks. This essentially reverts 2bcf6e97, but that is safe to do now that the quota sync code that was causing problems during long quotacheck executions is now gone. The reclaim work is currently protected from running during the unmount process by a check against MS_ACTIVE. Unfortunately, this also means that the reclaim work cannot run during mount. The unmount process should stop the reclaim cleanly before freeing anything that the reclaim work depends on, so there is no need to have this guard in place. Also, the inode reclaim work is demand driven, so there is no need to start it immediately during mount. It will be started the moment an inode is queued for reclaim, so qutoacheck will trigger it just fine. Signed-off-by: Dave Chinner <[email protected]> --- fs/xfs/xfs_super.c | 20 ++++++++++---------- fs/xfs/xfs_sync.c | 19 +++++++++---------- 2 files changed, 19 insertions(+), 20 deletions(-)
fs/xfs/xfs_super.c +10 -10
@@ -981,8 +981,6 @@ xfs_fs_put_super( {struct xfs_mount *mp =XFS_M (sb); -xfs_syncd_stop (mp); -/* * Blow away any referenced inode in the filestreams cache. * This can and will cause log traffic as inodes go inactive
@@ -993,6 +991,7 @@ xfs_fs_put_super(xfs_flush_buftarg (mp->m_ddev_targp,1 );xfs_unmountfs (mp); +xfs_syncd_stop (mp);xfs_freesb (mp);xfs_icsb_destroy_counters (mp);xfs_destroy_mount_workqueues (mp);
@@ -1362,31 +1361,33 @@ xfs_fs_fill_super( sb->s_time_gran =1 ;set_posix_acl_flag (sb); - error =xfs_mountfs (mp); + error =xfs_syncd_init (mp);if (error) goto out_filestream_unmount; - error =xfs_syncd_init (mp); + error =xfs_mountfs (mp);if (error) - goto out_unmount; + goto out_syncd_stop; root =igrab (VFS_I (mp->m_rootip));if (!root) { error =ENOENT ; - goto out_syncd_stop; + goto out_unmount; }if (is_bad_inode (root)) { error =EINVAL ; - goto out_syncd_stop; + goto out_unmount; } sb->s_root =d_make_root (root);if (!sb->s_root) { error =ENOMEM ; - goto out_syncd_stop; + goto out_unmount; }return 0 ; + out_syncd_stop: +xfs_syncd_stop (mp); out_filestream_unmount:xfs_filestream_unmount (mp); out_free_sb:
@@ -1403,8 +1404,6 @@ out_destroy_workqueues: out:return -error; - out_syncd_stop: -xfs_syncd_stop (mp); out_unmount:/* * Blow away any referenced inode in the filestreams cache.
@@ -1416,6 +1415,7 @@ out_destroy_workqueues:xfs_flush_buftarg (mp->m_ddev_targp,1 );xfs_unmountfs (mp); +xfs_syncd_stop (mp); goto out_free_sb; }
fs/xfs/xfs_sync.c +9 -10
@@ -460,7 +460,15 @@ xfs_sync_worker( struct xfs_mount, m_sync_work);int error; -if (!(mp->m_flags &XFS_MOUNT_RDONLY )) { +/* +* We shouldn't write/force the log if we are in the mount/unmount +* process or on a read only filesystem. The workqueue still needs to be +* active in both cases, however, because it is used for inode reclaim +* during these times. hence use the MS_ACTIVE flag to avoid doing +* anything in these periods. +*/ +if (!(mp->m_super->s_flags &MS_ACTIVE ) && + !(mp->m_flags &XFS_MOUNT_RDONLY )) {/* dgc: errors ignored here */ if (mp->m_super->s_frozen ==SB_UNFROZEN &&xfs_log_need_covered (mp))
@@ -488,14 +496,6 @@ xfs_syncd_queue_reclaim( struct xfs_mount *mp) { -/* -* We can have inodes enter reclaim after we've shut down the syncd -* workqueue during unmount, so don't allow reclaim work to be queued -* during unmount. -*/ -if (!(mp->m_super->s_flags &MS_ACTIVE )) -return ; -rcu_read_lock ();if (radix_tree_tagged (&mp->m_perag_tree,XFS_ICI_RECLAIM_TAG )) { queue_delayed_work(xfs_syncd_wq, &mp->m_reclaim_work,
@@ -564,7 +564,6 @@ xfs_syncd_init(INIT_DELAYED_WORK (&mp->m_reclaim_work, xfs_reclaim_worker);xfs_syncd_queue_sync (mp); -xfs_syncd_queue_reclaim (mp);return 0 ; }
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/8] xfs: initialise xfssync work before running quotachecks
In reply to Dave ChinnerQuoted 38 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Because the mount process can run a quotacheck and consume lots of inodes, we need to be able to run periodic inode reclaim during the mount process. This will prevent running the system out of memory during quota checks.
This essentially reverts
2bcf6e97, but that is safe to do now that the quota sync code that was causing problems during long quotacheck executions is now gone.Signed-off-by Dave Chinner <[email protected]>Reviewed-by Christoph Hellwig <[email protected]>--- fs/xfs/xfs_super.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-)fs/xfs/xfs_super.c +3
-3@@ -1346,31 +1346,33 @@ xfs_fs_fill_super( sb->s_time_gran =1 ;set_posix_acl_flag (sb); - error =xfs_mountfs (mp); + error =xfs_syncd_init (mp);if (error) goto out_filestream_unmount; - error =xfs_syncd_init (mp); + error =xfs_mountfs (mp);if (error) - goto out_unmount; + goto out_syncd_stop; root =igrab (VFS_I (mp->m_rootip));if (!root) {
fs/xfs/xfs_super.c xfs_fs_fill_super( · line 1366
Quoted 10 lines
fs/xfs/xfs_super.c +2
-2error =ENOENT ; - goto out_syncd_stop; + goto out_unmount; }if (is_bad_inode (root)) { error =EINVAL ; - goto out_syncd_stop; + goto out_unmount; } sb->s_root =d_make_root (root);
^^^^
I haven't seen a patch to use d_make_root instead of d_alloc_root, so we'll take the earlier version of this patch... this one doesn't apply. ;)
-Ben
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/8] xfs: initialise xfssync work before running quotachecks
Quoted 2 lines · Ben Myers
I haven't seen a patch to use
d_make_rootinstead ofd_alloc_root, so we'll take the earlier version of this patch... this one doesn't apply. ;)
d_alloc_root is gone in mainline. Maybe get the next pull request out without this patch, and then send it post -rc1 on a tree based upton rc1?
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/8] xfs: initialise xfssync work before running quotachecks
Quoted 7 lines · Christoph Hellwig
Quoted 2 lines · Ben Myers
I haven't seen a patch to use
d_make_rootinstead ofd_alloc_root, so we'll take the earlier version of this patch... this one doesn't apply. ;)
d_alloc_rootis gone in mainline. Maybe get the next pull request out without this patch, and then send it post -rc1 on a tree based upton rc1?
That sounds good to me.
-Ben
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 4/8 xfs: remove MS_ACTIVE guard from inode reclaim work
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
We need to be able to queue inode reclaim work during the mount process as quotacheck can cause large amounts of inodes to be read and we need to clean them up periodically as the shrinkers can not run until after the mount process has completed.
The reclaim work is currently protected from running during the unmount process by a check against MS_ACTIVE. Unfortunately, this also means that the relcaim work cannot run during mount. The unmount process should stop the reclaim cleanly before freeing anything that the reclaim work depends on, so there is no need to have this guard in place.
Also, the inode reclaim work is demand driven, so there is no need to start it immediately during mount. It will be started the moment an inode is queued for reclaim, so qutoacheck will trigger it just fine.
--- fs/xfs/xfs_super.c | 3 +-- fs/xfs/xfs_sync.c | 27 ++++++++++++++++----------- 2 files changed, 17 insertions(+), 13 deletions(-)
fs/xfs/xfs_super.c +1 -2
@@ -965,8 +965,6 @@ xfs_fs_put_super( {struct xfs_mount *mp =XFS_M (sb); -xfs_syncd_stop (mp); -/* * Blow away any referenced inode in the filestreams cache. * This can and will cause log traffic as inodes go inactive
@@ -977,6 +975,7 @@ xfs_fs_put_super(xfs_flush_buftarg (mp->m_ddev_targp,1 );xfs_unmountfs (mp); +xfs_syncd_stop (mp);xfs_freesb (mp);xfs_icsb_destroy_counters (mp);xfs_destroy_mount_workqueues (mp);
fs/xfs/xfs_sync.c +16 -11
@@ -460,7 +460,15 @@ xfs_sync_worker( struct xfs_mount, m_sync_work);int error; -if (!(mp->m_flags &XFS_MOUNT_RDONLY )) { +/* +* We shouldn't write/force the log if we are in the mount/unmount +* process or on a read only filesystem. The workqueue still needs to be +* active in both cases, however, because it is used for inode reclaim +* during these times. hence use the MS_ACTIVE flag to avoid doing +* anything in these periods. +*/ +if (!(mp->m_super->s_flags &MS_ACTIVE ) && + !(mp->m_flags &XFS_MOUNT_RDONLY )) {/* dgc: errors ignored here */ if (mp->m_super->s_frozen ==SB_UNFROZEN &&xfs_log_need_covered (mp))
@@ -488,14 +496,6 @@ xfs_syncd_queue_reclaim( struct xfs_mount *mp) { -/* -* We can have inodes enter reclaim after we've shut down the syncd -* workqueue during unmount, so don't allow reclaim work to be queued -* during unmount. -*/ -if (!(mp->m_super->s_flags &MS_ACTIVE )) -return ; -rcu_read_lock ();if (radix_tree_tagged (&mp->m_perag_tree,XFS_ICI_RECLAIM_TAG )) { queue_delayed_work(xfs_syncd_wq, &mp->m_reclaim_work,
@@ -564,7 +564,6 @@ xfs_syncd_init(INIT_DELAYED_WORK (&mp->m_reclaim_work, xfs_reclaim_worker);xfs_syncd_queue_sync (mp); -xfs_syncd_queue_reclaim (mp);return 0 ; }
@@ -574,7 +573,13 @@ xfs_syncd_stop( struct xfs_mount *mp) {cancel_delayed_work_sync (&mp->m_sync_work); -cancel_delayed_work_sync (&mp->m_reclaim_work); + +/* +* we flush any pending inode reclaim work rather than cancel it here. +* This ensures that there are no clean inodes queued during unmount +* left unreclaimed when we return. +*/ +flush_delayed_work_sync (&mp->m_reclaim_work);cancel_work_sync (&mp->m_flush_work); }
Signature · 6 lines
1.7.9 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 5/8 xfs: don't cache inodes read through bulkstat
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
When we read inodes via bulkstat, we generally only read them once and then throw them away - they never get used again. If we retain them in cache, then it simply causes the working set of inodes and other cached items to be reclaimed just so the inode cache can grow.
Avoid this problem by marking inodes read by bulkstat not to be cached and check this flag in .drop_inode to determine whether the inode should be added to the VFS LRU or not. If the inode lookup hits an already cached inode, then don't set the flag. If the inode lookup hits an inode marked with no cache flag, remove the flag and allow it to be cached once the current reference goes away.
Inodes marked as not cached will get cleaned up by the background inode reclaim or via memory pressure, so they will still generate some short term cache pressure. They will, however, be reclaimed much sooner and in preference to cache hot inodes.
--- fs/xfs/xfs_iget.c | 8 ++++++-- fs/xfs/xfs_inode.h | 4 +++- fs/xfs/xfs_itable.c | 3 ++- fs/xfs/xfs_super.c | 17 +++++++++++++++++ 4 files changed, 28 insertions(+), 4 deletions(-)
fs/xfs/xfs_iget.c +6 -2
@@ -289,7 +289,7 @@ xfs_iget_cache_hit(if (lock_flags !=0 )xfs_ilock (ip, lock_flags); -xfs_iflags_clear (ip,XFS_ISTALE ); +xfs_iflags_clear (ip,XFS_ISTALE |XFS_IDONTCACHE );XFS_STATS_INC (xs_ig_found);return 0 ;
@@ -314,6 +314,7 @@ xfs_iget_cache_miss(struct xfs_inode *ip;int error;xfs_agino_t agino =XFS_INO_TO_AGINO (mp, ino); +int iflags; ip =xfs_inode_alloc (mp, ino);if (!ip)
@@ -358,8 +359,11 @@ xfs_iget_cache_miss(* memory barrier that ensures this detection works correctly at lookup * time. */ + iflags =XFS_INEW ; +if (flags &XFS_IGET_DONTCACHE ) + iflags |=XFS_IDONTCACHE ; ip->i_udquot = ip->i_gdquot =NULL ; -xfs_iflags_set (ip,XFS_INEW ); +xfs_iflags_set (ip, iflags);/* insert the new inode */ spin_lock (&pag->pag_ici_lock);
fs/xfs/xfs_inode.h +3 -1
@@ -387,10 +387,11 @@ xfs_set_projid(struct xfs_inode *ip,#define XFS_IFLOCK (1 << __XFS_IFLOCK_BIT)#define __XFS_IPINNED_BIT 8/* wakeup key for zero pin count */ #define XFS_IPINNED (1 << __XFS_IPINNED_BIT) +#define XFS_IDONTCACHE (1 << 9)/* don't cache the inode long term */ /* * Per-lifetime flags need to be reset when re-using a reclaimable inode during -* inode lookup. Thi prevents unintended behaviour on the new inode from +* inode lookup. This prevents unintended behaviour on the new inode from * ocurring. */ #define XFS_IRECLAIM_RESET_FLAGS \
@@ -553,6 +554,7 @@ do { \*/ #define XFS_IGET_CREATE 0x1#define XFS_IGET_UNTRUSTED 0x2 +#define XFS_IGET_DONTCACHE 0x4 int xfs_inotobp(struct xfs_mount *, struct xfs_trans *, xfs_ino_t, struct xfs_dinode **,
fs/xfs/xfs_itable.c +2 -1
@@ -75,7 +75,8 @@ xfs_bulkstat_one_int(return XFS_ERROR (ENOMEM ); error =xfs_iget (mp,NULL , ino, -XFS_IGET_UNTRUSTED ,XFS_ILOCK_SHARED , &ip); + (XFS_IGET_DONTCACHE |XFS_IGET_UNTRUSTED ), +XFS_ILOCK_SHARED , &ip);if (error) { *stat =BULKSTAT_RV_NOTHING ; goto out_free;
fs/xfs/xfs_super.c +17 -0
@@ -950,6 +950,22 @@ xfs_fs_evict_inode(xfs_inactive (ip); } +/* +* We do an unlocked check for XFS_IDONTCACHE here because we are already +* serialised against cache hits here via the inode->i_lock and igrab() in +* xfs_iget_cache_hit(). Hence a lookup that might clear this flag will not be +* racing with us, and it avoids needing to grab a spinlock here for every inode +* we drop the final reference on. +*/ +STATIC int +xfs_fs_drop_inode ( +struct inode *inode) +{ +struct xfs_inode *ip =XFS_I (inode); + +return generic_drop_inode (inode) || (ip->i_flags &XFS_IDONTCACHE ); +} + STATIC voidxfs_free_fsname (struct xfs_mount *mp)
@@ -1433,6 +1449,7 @@ static const struct super_operations xfs_super_operations = { .destroy_inode = xfs_fs_destroy_inode, .dirty_inode = xfs_fs_dirty_inode, .evict_inode = xfs_fs_evict_inode, + .drop_inode = xfs_fs_drop_inode, .put_super = xfs_fs_put_super, .sync_fs = xfs_fs_sync_fs, .freeze_fs = xfs_fs_freeze,
Signature · 6 lines
1.7.9 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 6/8 xfs: Account log unmount transaction correctly
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
There have been a few reports of this warning appearing recently:
XFS (dm-4): xlog_space_left: head behind tail
tail_cycle = 129, tail_bytes = 20163072
GH cycle = 129, GH bytes = 20162880
The common cause appears to be lots of freeze and unfreeze cycles, and the output from the warnings indicates that we are leaking around 8 bytes of log space per freeze/unfreeze cycle.
When we freeze the filesystem, we write an unmount record and that uses xlog_write directly - a special type of transaction, effectively. What it doesn't do, however, is correctly acocunt for the log space it uses. The unmount record writes an 8 byte structure with a special magic number into the log, and the space this consumes is not accounted for in the log ticket tracking the operation. Hence we leak 8 bytes every unmount record that is written.
--- fs/xfs/xfs_log.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
fs/xfs/xfs_log.c +2 -1
@@ -726,8 +726,9 @@ xfs_log_unmount_write(xfs_mount_t *mp) .lv_iovecp = ®, }; -/* remove inited flag */ +/* remove inited flag, and account for space used */ tic->t_flags =0 ; + tic->t_curr_res -=sizeof (magic); error =xlog_write (log, &vec, tic, &lsn,NULL ,XLOG_UNMOUNT_TRANS );/*
Signature · 6 lines
1.7.9 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 6/8] xfs: Account log unmount transaction correctly
Looks good.
Can we add a testcase doing lots of freeze/unfreeze cycles with no other load to xftests to verify the fix?
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 6/8] xfs: Account log unmount transaction correctly
Quoted 4 lines · Christoph Hellwig
Looks good.
Can we add a testcase doing lots of freeze/unfreeze cycles with no other load to xftests to verify the fix?
It's just a trivial while loop that needs about a hundred freeze/unfreeze ops to trigger on a 10GB filesystem, so I'll add a test for that...
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 6/8] xfs: Account log unmount transaction correctly
In reply to Dave ChinnerQuoted 21 lines · Dave Chinner
From: Dave Chinner <[email protected]>
There have been a few reports of this warning appearing recently:
XFS (dm-4):
xlog_space_left: head behind tail
tail_cycle= 129,tail_bytes= 20163072
GH cycle = 129, GH bytes = 20162880The common cause appears to be lots of freeze and unfreeze cycles, and the output from the warnings indicates that we are leaking around 8 bytes of log space per freeze/unfreeze cycle.
When we freeze the filesystem, we write an unmount record and that uses
xlog_writedirectly - a special type of transaction, effectively. What it doesn't do, however, is correctly acocunt for the log space it uses. The unmount record writes an 8 byte structure with a special magic number into the log, and the space this consumes is not accounted for in the log ticket tracking the operation. Hence we leak 8 bytes every unmount record that is written.
Quoted 1 line
Signed-off-by Dave Chinner <[email protected]>
Looks good.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 7/8 xfs: fix fstrim offset calculations
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
xfs_ioc_fstrim() doesn't treat the incoming offset and length correctly. It treats them as a filesystem block address, rather than a disk address. This is wrong because the range passed in is a linear representation , while the filesystem block address notiation is a sparse representation. Hence we cannot convert the range direct to filesystem block units and then use that for calculating the range to trim.
While this sounds dangerous, the problem is limited to calculting what AGs need to be trimmed. The code that calcuates the actual ranges to trim gets the right result (i.e. only ever discards free space), even though it uses the wrong ranges to limit what is trimmed. Hence this is not a bug that endangers user data.
Fix this by treating the range as a disk address range and use the appropriate functions to convert the range into the desired formats for calculations.
Further, fix the first free extent lookup (the longest) to actually find the largest free extent. Currently this lookup uses a <= lookup, which results in finding the extent to the left of the largest because we can never get an exact match on the largest extent. This is due to the fact that while we know it's size, we don't know it's location and so the exact match fails and we move one record to the left to get the next largest extent. Instead, use a >= search so that the lookup returns the largest extent regardless of the fact we don't get an exact match on it.
--- fs/xfs/xfs_alloc.c | 2 +- fs/xfs/xfs_alloc.h | 7 +++++ fs/xfs/xfs_discard.c | 61 +++++++++++++++++++++++++++++++------------------ 3 files changed, 46 insertions(+), 24 deletions(-)
fs/xfs/xfs_alloc.c +1 -1
@@ -69,7 +69,7 @@ xfs_alloc_lookup_eq(* Lookup the first record greater than or equal to [bno, len] * in the btree given by cur. */ -STATIC int /* error */ +int /* error */ xfs_alloc_lookup_ge( struct xfs_btree_cur *cur, /* btree cursor */ xfs_agblock_t bno, /* starting block of extent */
fs/xfs/xfs_alloc.h +7 -0
@@ -248,6 +248,13 @@ xfs_alloc_lookup_le(xfs_extlen_t len,/* length of extent */ int *stat); /* success/failure */ +int /* error */ +xfs_alloc_lookup_ge ( +struct xfs_btree_cur *cur,/* btree cursor */ +xfs_agblock_t bno,/* starting block of extent */ +xfs_extlen_t len,/* length of extent */ +int *stat);/* success/failure */ + int /* error */ xfs_alloc_get_rec( struct xfs_btree_cur *cur, /* btree cursor */
fs/xfs/xfs_discard.c +38 -23
124 lines not on this page · raw message
Signature · 6 lines
1.7.9 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 7/8] xfs: fix fstrim offset calculations
Quoted 30 lines · Dave Chinner
From: Dave Chinner <[email protected]>
xfs_ioc_fstrim()doesn't treat the incoming offset and length correctly. It treats them as a filesystem block address, rather than a disk address. This is wrong because the range passed in is a linear representation , while the filesystem block address notiation is a sparse representation. Hence we cannot convert the range direct to filesystem block units and then use that for calculating the range to trim.While this sounds dangerous, the problem is limited to calculting what AGs need to be trimmed. The code that calcuates the actual ranges to trim gets the right result (i.e. only ever discards free space), even though it uses the wrong ranges to limit what is trimmed. Hence this is not a bug that endangers user data.
Fix this by treating the range as a disk address range and use the appropriate functions to convert the range into the desired formats for calculations.
Further, fix the first free extent lookup (the longest) to actually find the largest free extent. Currently this lookup uses a <= lookup, which results in finding the extent to the left of the largest because we can never get an exact match on the largest extent. This is due to the fact that while we know it's size, we don't know it's location and so the exact match fails and we move one record to the left to get the next largest extent. Instead, use a >= search so that the lookup returns the largest extent regardless of the fact we don't get an exact match on it.
Quoted 1 line
Signed-off-by Dave Chinner <[email protected]>
Looks good with the minor nitpick below.
fs/xfs/xfs_discard.c xfs_trim_extents( · line 71
Quoted 3 lines
fs/xfs/xfs_discard.c +1
-1- error =xfs_alloc_lookup_le (cur,0 , + error =xfs_alloc_lookup_ge (cur,0 ,be32_to_cpu (XFS_BUF_TO_AGF (agbp)->agf_longest), &i);
After this change xfs_alloc_lookup_le can be marked static in xfs_alloc.c
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 7/8] xfs: fix fstrim offset calculations
In reply to Dave ChinnerQuoted 10 lines · Dave Chinner
From: Dave Chinner <[email protected]>
xfs_ioc_fstrim()doesn't treat the incoming offset and length correctly. It treats them as a filesystem block address, rather than a disk address. This is wrong because the range passed in is a linear representation , while the filesystem block address notiation is a sparse representation. Hence we cannot convert the range direct to filesystem block units and then use that for calculating the range to trim.
Quoted 5 lines
While this sounds dangerous, the problem is limited to calculting what AGs need to be trimmed. The code that calcuates the actual ranges to trim gets the right result (i.e. only ever discards free space), even though it uses the wrong ranges to limit what is trimmed. Hence this is not a bug that endangers user data.
Yep, I can see that the calculation of what we pass to blkdev_issue_discard() is correct and always a free extent. I am having a hard time seeing the problem related to calculating which AGs to trim. Can you give an example?
Quoted 14 lines
Fix this by treating the range as a disk address range and use the appropriate functions to convert the range into the desired formats for calculations.
Further, fix the first free extent lookup (the longest) to actually find the largest free extent. Currently this lookup uses a <= lookup, which results in finding the extent to the left of the largest because we can never get an exact match on the largest extent. This is due to the fact that while we know it's size, we don't know it's location and so the exact match fails and we move one record to the left to get the next largest extent. Instead, use a >= search so that the lookup returns the largest extent regardless of the fact we don't get an exact match on it.
Quoted 1 line
Signed-off-by Dave Chinner <[email protected]>
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 7/8] xfs: fix fstrim offset calculations
Quoted 17 lines · Ben Myers
Quoted 10 lines · Dave Chinner
From: Dave Chinner <[email protected]>
xfs_ioc_fstrim()doesn't treat the incoming offset and length correctly. It treats them as a filesystem block address, rather than a disk address. This is wrong because the range passed in is a linear representation , while the filesystem block address notiation is a sparse representation. Hence we cannot convert the range direct to filesystem block units and then use that for calculating the range to trim.Quoted 5 lines
While this sounds dangerous, the problem is limited to calculting what AGs need to be trimmed. The code that calcuates the actual ranges to trim gets the right result (i.e. only ever discards free space), even though it uses the wrong ranges to limit what is trimmed. Hence this is not a bug that endangers user data.
Quoted 3 lines
Yep, I can see that the calculation of what we pass to
blkdev_issue_discard()is correct and always a free extent. I am having a hard time seeing the problem related to calculating which AGs to trim. Can you give an example?
I don't have the debug traces anymore, but the problem is this sort of thing. Take a 80MB filesystem with 4 AGs, each AG is 20MB, which is ~5000 filesystem blocks. That means we need 13 bits to store the block count per AG. i.e. agblklogi = 13. Now, the FSB addressing format is sparse, and the calculation is this:
FSBNO = (AGNO << agblklog) | AGBNO
Note the terminology? FSBNO != FSB. FSB is just a range converted to filesystem block units. FSBNO is the filesystem block number, an address.
offset offset + length
+-------------------------------------------+
range: 0 80MB daddr: 0 160k FSB: 0 20k
AG: +----------+----------+----------+----------+
0 1 2 3
AGBNO: 0 5k
0 5k
0 5k
0 5k
FSBNO: 0 5k
8k 13k
16k 21k
24k 29kIOWs, the FSBNO range looks like this:
+----------+ +----------+ +----------+ +----------+
0 5k 8k 13k 16k 21k 24k 29kAnd there are regions that are simple invalid (the empty, sparse bits). This is done to make all the mathematics easy within each AG as you can convert from the FSBNO straight to the AGBNO (and vice versa) without needing to know the address of the first block of the AG. It means it is easy for AGs to manage their own space without needing to care about where they exist in the larger disk address space - that is complete abstracted away from the internal freespace and inode management as they all use AGBNO notation ot reference blocks within the AG.
As a result, the FSBNO range of the filesystem is quite a bit larger than the FSB range of the filesystem. So, if we trim a byte range of 0 to 80MB, but treat that as a FSBNO and then convert it to an AGNO, 80MB = 20k FSBs = AG 2.
Hence rather than trimming the entire range of AGs (0-3), we trim 0-2. Hence we need to convert the byte range to a daddr range, and from there extract the AGNO according to FSBNO encoding.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 8/8 xfs: add lots of attribute trace points
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
--- fs/xfs/xfs_attr.c | 16 ++++++++++++ fs/xfs/xfs_attr_leaf.c | 40 +++++++++++++++++++++++++++--- fs/xfs/xfs_da_btree.c | 32 ++++++++++++++++++++++++ fs/xfs/xfs_trace.h | 62 ++++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 144 insertions(+), 6 deletions(-)
fs/xfs/xfs_attr.c +16 -0
@@ -853,6 +853,8 @@ xfs_attr_shortform_addname(xfs_da_args_t *args) {int newsize, forkoff, retval; +trace_xfs_attr_sf_addname (args); + retval =xfs_attr_shortform_lookup (args);if ((args->flags &ATTR_REPLACE ) && (retval ==ENOATTR )) {return (retval);
@@ -896,6 +898,8 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)xfs_dabuf_t *bp;int retval, error, committed, forkoff; +trace_xfs_attr_leaf_addname (args); +/* * Read the (only) block in the attribute list in. */
@@ -920,6 +924,9 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)xfs_da_brelse (args->trans, bp);return (retval); } + +trace_xfs_attr_leaf_replace (args); + args->op_flags |=XFS_DA_OP_RENAME ;/* an atomic rename */ args->blkno2 = args->blkno;/* set 2nd entry info*/ args->index2 = args->index;
@@ -1090,6 +1097,8 @@ xfs_attr_leaf_removename(xfs_da_args_t *args)xfs_dabuf_t *bp;int error, committed, forkoff; +trace_xfs_attr_leaf_removename (args); +/* * Remove the attribute. */
@@ -1223,6 +1232,8 @@ xfs_attr_node_addname(xfs_da_args_t *args)xfs_mount_t *mp;int committed, retval, error; +trace_xfs_attr_node_addname (args); +/* * Fill in bucket of arguments/results/context to carry around. */
@@ -1249,6 +1260,9 @@ restart: }else if (retval ==EEXIST ) {if (args->flags &ATTR_CREATE ) goto out; + +trace_xfs_attr_node_replace (args); + args->op_flags |=XFS_DA_OP_RENAME ;/* atomic rename op */ args->blkno2 = args->blkno;/* set 2nd entry info*/ args->index2 = args->index;
@@ -1480,6 +1494,8 @@ xfs_attr_node_removename(xfs_da_args_t *args)xfs_dabuf_t *bp;int retval, error, committed, forkoff; +trace_xfs_attr_node_removename (args); +/* * Tie a string around our finger to remind us where we are. */
fs/xfs/xfs_attr_leaf.c +36 -4
@@ -235,6 +235,8 @@ xfs_attr_shortform_create(xfs_da_args_t *args)xfs_inode_t *dp;xfs_ifork_t *ifp; +trace_xfs_attr_sf_create (args); + dp = args->dp;ASSERT (dp !=NULL ); ifp = dp->i_afp;
@@ -268,6 +270,8 @@ xfs_attr_shortform_add(xfs_da_args_t *args, int forkoff)xfs_inode_t *dp;xfs_ifork_t *ifp; +trace_xfs_attr_sf_add (args); + dp = args->dp; mp = dp->i_mount; dp->i_d.di_forkoff = forkoff;
@@ -337,6 +341,8 @@ xfs_attr_shortform_remove(xfs_da_args_t *args)xfs_mount_t *mp;xfs_inode_t *dp; +trace_xfs_attr_sf_remove (args); + dp = args->dp; mp = dp->i_mount; base =sizeof (xfs_attr_sf_hdr_t);
@@ -405,6 +411,8 @@ xfs_attr_shortform_lookup(xfs_da_args_t *args)int i;xfs_ifork_t *ifp; +trace_xfs_attr_sf_lookup (args); + ifp = args->dp->i_afp;ASSERT (ifp->if_flags &XFS_IFINLINE ); sf = (xfs_attr_shortform_t *)ifp->if_u1.if_data;
@@ -476,6 +484,8 @@ xfs_attr_shortform_to_leaf(xfs_da_args_t *args)xfs_dabuf_t *bp;xfs_ifork_t *ifp; +trace_xfs_attr_sf_to_leaf (args); + dp = args->dp; ifp = dp->i_afp; sf = (xfs_attr_shortform_t *)ifp->if_u1.if_data;
@@ -775,6 +785,8 @@ xfs_attr_leaf_to_shortform(xfs_dabuf_t *bp, xfs_da_args_t *args, int forkoff)char *tmpbuffer;int error, i; +trace_xfs_attr_leaf_to_sf (args); + dp = args->dp; tmpbuffer =kmem_alloc (XFS_LBSIZE (dp->i_mount),KM_SLEEP );ASSERT (tmpbuffer !=NULL );
@@ -848,6 +860,8 @@ xfs_attr_leaf_to_node(xfs_da_args_t *args)xfs_dablk_t blkno;int error; +trace_xfs_attr_leaf_to_node (args); + dp = args->dp; bp1 = bp2 =NULL ; error =xfs_da_grow_inode (args, &blkno);
@@ -911,6 +925,8 @@ xfs_attr_leaf_create(xfs_da_args_t *args, xfs_dablk_t blkno, xfs_dabuf_t **bpp)xfs_dabuf_t *bp;int error; +trace_xfs_attr_leaf_create (args); + dp = args->dp;ASSERT (dp !=NULL ); error = xfs_da_get_buf(args->trans, args->dp, blkno, -1, &bp,
@@ -948,6 +964,8 @@ xfs_attr_leaf_split(xfs_da_state_t *state, xfs_da_state_blk_t *oldblk,xfs_dablk_t blkno;int error; +trace_xfs_attr_leaf_split (state->args); +/* * Allocate space for a new leaf node. */
@@ -977,10 +995,13 @@ xfs_attr_leaf_split(xfs_da_state_t *state, xfs_da_state_blk_t *oldblk,* * Insert the "new" entry in the correct block. */ -if (state->inleaf) +if (state->inleaf) { +trace_xfs_attr_leaf_add_old (state->args); error =xfs_attr_leaf_add (oldblk->bp, state->args); -else + }else { +trace_xfs_attr_leaf_add_new (state->args); error =xfs_attr_leaf_add (newblk->bp, state->args); + }/* * Update last hashval in each block since we added the name.
@@ -1001,6 +1022,8 @@ xfs_attr_leaf_add(xfs_dabuf_t *bp, xfs_da_args_t *args)xfs_attr_leaf_map_t *map;int tablesize, entsize, sum, tmp, i; +trace_xfs_attr_leaf_add (args); + leaf = bp->data;ASSERT (leaf->hdr.info.magic ==cpu_to_be16 (XFS_ATTR_LEAF_MAGIC )); ASSERT((args->index >= 0)
@@ -1128,8 +1151,6 @@ xfs_attr_leaf_add_work(xfs_dabuf_t *bp, xfs_da_args_t *args, int mapindex) (be32_to_cpu(entry->hashval) <= be32_to_cpu((entry+1)->hashval)));/* -* Copy the attribute name and value into the new space. -* * For "remote" attribute values, simply note that we need to * allocate space for the "remote" value. We can't actually * allocate the extents in this transaction, and we can't decide
@@ -1265,6 +1286,8 @@ xfs_attr_leaf_rebalance(xfs_da_state_t *state, xfs_da_state_blk_t *blk1,ASSERT (leaf2->hdr.info.magic ==cpu_to_be16 (XFS_ATTR_LEAF_MAGIC )); args = state->args; +trace_xfs_attr_leaf_rebalance (args); +/* * Check ordering of blocks, reverse if it makes things simpler. *
@@ -1810,6 +1833,8 @@ xfs_attr_leaf_unbalance(xfs_da_state_t *state, xfs_da_state_blk_t *drop_blk,xfs_mount_t *mp;char *tmpbuffer; +trace_xfs_attr_leaf_unbalance (state->args); +/* * Set up environment. */
@@ -1919,6 +1944,8 @@ xfs_attr_leaf_lookup_int(xfs_dabuf_t *bp, xfs_da_args_t *args)int probe, span;xfs_dahash_t hashval; +trace_xfs_attr_leaf_lookup (args); + leaf = bp->data;ASSERT (leaf->hdr.info.magic ==cpu_to_be16 (XFS_ATTR_LEAF_MAGIC )); ASSERT(be16_to_cpu(leaf->hdr.count)
@@ -2445,6 +2472,7 @@ xfs_attr_leaf_clearflag(xfs_da_args_t *args)char *name;#endif /* DEBUG */ +trace_xfs_attr_leaf_clearflag (args);/* * Set up the operation. */
@@ -2509,6 +2537,8 @@ xfs_attr_leaf_setflag(xfs_da_args_t *args)xfs_dabuf_t *bp;int error; +trace_xfs_attr_leaf_setflag (args); +/* * Set up the operation. */
@@ -2565,6 +2595,8 @@ xfs_attr_leaf_flipflags(xfs_da_args_t *args)char *name1, *name2;#endif /* DEBUG */ +trace_xfs_attr_leaf_flipflags (args); +/* * Read the block containing the "old" attr */
fs/xfs/xfs_da_btree.c +32 -0
@@ -108,6 +108,8 @@ xfs_da_node_create(xfs_da_args_t *args, xfs_dablk_t blkno, int level,int error;xfs_trans_t *tp; +trace_xfs_da_node_create (args); + tp = args->trans; error =xfs_da_get_buf (tp, args->dp, blkno,-1 , &bp, whichfork);if (error)
@@ -140,6 +142,8 @@ xfs_da_split(xfs_da_state_t *state)xfs_dabuf_t *bp;int max, action, error, i; +trace_xfs_da_split (state->args); +/* * Walk back up the tree splitting/inserting/adjusting as necessary. * If we need to insert and there isn't room, split the node, then
@@ -178,10 +182,12 @@ xfs_da_split(xfs_da_state_t *state) state->extravalid =1 ;if (state->inleaf) { state->extraafter =0 ;/* before newblk */ +trace_xfs_attr_leaf_split_before (state->args); error =xfs_attr_leaf_split (state, oldblk, &state->extrablk); }else { state->extraafter =1 ;/* after newblk */ +trace_xfs_attr_leaf_split_after (state->args); error =xfs_attr_leaf_split (state, newblk, &state->extrablk); }
@@ -300,6 +306,8 @@ xfs_da_root_split(xfs_da_state_t *state, xfs_da_state_blk_t *blk1,xfs_mount_t *mp;xfs_dir2_leaf_t *leaf; +trace_xfs_da_root_split (state->args); +/* * Copy the existing (incorrect) block from the root node position * to a free space somewhere.
@@ -380,6 +388,8 @@ xfs_da_node_split(xfs_da_state_t *state, xfs_da_state_blk_t *oldblk,int newcount, error;int useextra; +trace_xfs_da_node_split (state->args); + node = oldblk->bp->data;ASSERT (node->hdr.info.magic ==cpu_to_be16 (XFS_DA_NODE_MAGIC ));
@@ -466,6 +476,8 @@ xfs_da_node_rebalance(xfs_da_state_t *state, xfs_da_state_blk_t *blk1,int count, tmp;xfs_trans_t *tp; +trace_xfs_da_node_rebalance (state->args); + node1 = blk1->bp->data; node2 = blk2->bp->data;/*
@@ -574,6 +586,8 @@ xfs_da_node_add(xfs_da_state_t *state, xfs_da_state_blk_t *oldblk,xfs_da_node_entry_t *btree;int tmp; +trace_xfs_da_node_add (state->args); + node = oldblk->bp->data;ASSERT (node->hdr.info.magic ==cpu_to_be16 (XFS_DA_NODE_MAGIC ));ASSERT ((oldblk->index >=0 ) && (oldblk->index <=be16_to_cpu (node->hdr.count)));
@@ -619,6 +633,8 @@ xfs_da_join(xfs_da_state_t *state)xfs_da_state_blk_t *drop_blk, *save_blk;int action, error; +trace_xfs_da_join (state->args); + action =0 ; drop_blk = &state->path.blk[ state->path.active-1 ]; save_blk = &state->altpath.blk[ state->path.active-1 ];
@@ -723,6 +739,8 @@ xfs_da_root_join(xfs_da_state_t *state, xfs_da_state_blk_t *root_blk)xfs_dabuf_t *bp;int error; +trace_xfs_da_root_join (state->args); + args = state->args;ASSERT (args !=NULL );ASSERT (root_blk->magic ==XFS_DA_NODE_MAGIC );
@@ -941,6 +959,8 @@ xfs_da_node_remove(xfs_da_state_t *state, xfs_da_state_blk_t *drop_blk)xfs_da_node_entry_t *btree;int tmp; +trace_xfs_da_node_remove (state->args); + node = drop_blk->bp->data;ASSERT (drop_blk->index <be16_to_cpu (node->hdr.count));ASSERT (drop_blk->index >=0 );
@@ -984,6 +1004,8 @@ xfs_da_node_unbalance(xfs_da_state_t *state, xfs_da_state_blk_t *drop_blk,int tmp;xfs_trans_t *tp; +trace_xfs_da_node_unbalance (state->args); + drop_node = drop_blk->bp->data; save_node = save_blk->bp->data;ASSERT (drop_node->hdr.info.magic ==cpu_to_be16 (XFS_DA_NODE_MAGIC ));
@@ -1230,6 +1252,7 @@ xfs_da_blk_link(xfs_da_state_t *state, xfs_da_state_blk_t *old_blk,/* * Link new block in before existing block. */ +trace_xfs_da_link_before (args); new_info->forw =cpu_to_be32 (old_blk->blkno); new_info->back = old_info->back;if (old_info->back) {
@@ -1251,6 +1274,7 @@ xfs_da_blk_link(xfs_da_state_t *state, xfs_da_state_blk_t *old_blk,/* * Link new block in after existing block. */ +trace_xfs_da_link_after (args); new_info->forw = old_info->forw; new_info->back =cpu_to_be32 (old_blk->blkno);if (old_info->forw) {
@@ -1348,6 +1372,7 @@ xfs_da_blk_unlink(xfs_da_state_t *state, xfs_da_state_blk_t *drop_blk,* Unlink the leaf block from the doubly linked chain of leaves. */ if (be32_to_cpu (save_info->back) == drop_blk->blkno) { +trace_xfs_da_unlink_back (args); save_info->back = drop_info->back;if (drop_info->back) { error = xfs_da_read_buf(args->trans, args->dp,
@@ -1365,6 +1390,7 @@ xfs_da_blk_unlink(xfs_da_state_t *state, xfs_da_state_blk_t *drop_blk,xfs_da_buf_done (bp); } }else { +trace_xfs_da_unlink_forward (args); save_info->forw = drop_info->forw;if (drop_info->forw) { error = xfs_da_read_buf(args->trans, args->dp,
@@ -1652,6 +1678,8 @@ xfs_da_grow_inode(int count;int error; +trace_xfs_da_grow_inode (args); +if (args->whichfork ==XFS_DATA_FORK ) { bno = args->dp->i_mount->m_dirleafblk; count = args->dp->i_mount->m_dirblkfsbs;
@@ -1690,6 +1718,8 @@ xfs_da_swap_lastblock(xfs_da_args_t *args, xfs_dablk_t *dead_blknop,xfs_dir2_leaf_t *dead_leaf2;xfs_dahash_t dead_hash; +trace_xfs_da_swap_lastblock (args); + dead_buf = *dead_bufp; dead_blkno = *dead_blknop; tp = args->trans;
@@ -1878,6 +1908,8 @@ xfs_da_shrink_inode(xfs_da_args_t *args, xfs_dablk_t dead_blkno,xfs_trans_t *tp;xfs_mount_t *mp; +trace_xfs_da_shrink_inode (args); + dp = args->dp; w = args->whichfork; tp = args->trans;
fs/xfs/xfs_trace.h +60 -2
80 lines not on this page · raw message
Signature · 6 lines
1.7.9 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 8/8] xfs: add lots of attribute trace points
Quoted 3 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Signed-off-by Dave Chinner <[email protected]>
Looks good mostly good, minor comments below:
fs/xfs/xfs_attr_leaf.c xfs_attr_leaf_add_work(xfs_dabuf_t *bp, xfs_da_args_t *args, int mapindex) · line 1132
Quoted 3 lines
fs/xfs/xfs_attr_leaf.c +0
-2/* -* Copy the attribute name and value into the new space. -*
Why does this get removed?
Quoted 7 lines
#define
DEFINE_DIR2_EVENT(name)\fs/xfs/xfs_trace.h +1
-1-DEFINE_EVENT (xfs_dir2_class, name, \ +DEFINE_EVENT (xfs_da_class, name, \ TP_PROTO(struct xfs_da_args *args), \TP_ARGS (args))
fs/xfs/xfs_trace.h DEFINE_DIR2_EVENT(xfs_dir2_node_replace); · line 1514
Quoted 10 lines
fs/xfs/xfs_trace.h +8
-0+#define DEFINE_ATTR_EVENT (name) \ +DEFINE_EVENT(xfs_da_class, name, \ + TP_PROTO(struct xfs_da_args *args), \ + TP_ARGS(args)) +#define DEFINE_DA_EVENT (name) \ +DEFINE_EVENT(xfs_da_class, name, \ + TP_PROTO(struct xfs_da_args *args), \ + TP_ARGS(args))
We should use the DEFINE_DA_EVENT macro for all of these tracepoints, instead of having two duplicates of it.
Also all the tracepoints should print of which fork we operate on, which is useful at least for the ones in common code.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 8/8] xfs: add lots of attribute trace points
In reply to Dave ChinnerQuoted 1 line · Dave Chinner
+DEFINE_ATTR_EVENT(xfs_attr_sf_removename);
This one is unused ^^^
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 8/8] xfs: add lots of attribute trace points
Quoted 4 lines · Ben Myers
Quoted 1 line · Dave Chinner
+DEFINE_ATTR_EVENT(xfs_attr_sf_removename);
This one is unused ^^^
An oversight due to copy-n-paste enthusiasm. Just remove it, or do you want me to send a new patch with it removed?
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 8/8] xfs: add lots of attribute trace points
Quoted 8 lines · Dave Chinner
Quoted 4 lines · Ben Myers
Quoted 1 line · Dave Chinner
+DEFINE_ATTR_EVENT(xfs_attr_sf_removename);
This one is unused ^^^
An oversight due to copy-n-paste enthusiasm. Just remove it, or do you want me to send a new patch with it removed?
I can remove it, thanks.
-Ben
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs