Re: [patch] Fix xfs_iunpin() sets I_DIRTY_SYNC after clear_inode().

Takenori Nagano
2006-10-12T12:22:21Z

Hi David,

I tried those patches, but they caused degradation.
These are results of "vmstat 10" while my test program was running.

- Before applying those patches:
procs -----------memory---------- ---swap-- -----io---- -system-- -----cpu------
r b swpd free buff cache si so bi bo in cs us sy id wa st
7 0 0 2768240 37632 210512 0 0 7 43367 268 676 1 49 50 0 0
9 0 0 2716352 37632 210672 0 0 0 362864 2154 47915 1 51 48 0 0
9 0 0 2663136 37664 210048 0 0 0 361745 2154 48258 1 50 49 0 0
10 0 0 2610688 37664 211184 0 0 0 360908 2152 48068 1 51 49 0 0
9 0 0 2557904 37680 210512 0 0 0 360254 2154 49036 1 51 48 0 0
10 0 0 2504832 37696 210304 0 0 0 362525 2153 48460 1 50 49 0 0

- After applying those patches:
procs -----------memory---------- ---swap-- -----io---- -system-- -----cpu------
r b swpd free buff cache si so bi bo in cs us sy id wa st
0 0 0 15584608 21776 153072 0 0 69 403 256 394 1 3 95 1 0
0 0 0 15586032 21824 153024 0 0 1 2319 2161 2944 0 2 98 0 0
1 0 0 15585920 21824 153104 0 0 0 2342 2161 2951 0 2 98 0 0
0 0 0 15585696 21824 152976 0 0 0 2364 2160 2978 0 2 98 0 0
1 0 0 15585360 21824 153168 0 0 0 2380 2161 3027 0 2 98 0 0
0 0 0 15585248 21824 152976 0 0 0 2348 2161 2983 0 2 98 0 0

Block I/O performance degradation was very serious.
Now, I am trying to ease the degradation.
Do you have any idea for resolving the degradation?

By the way, I found some mistakes in your patch.
Please correct them.

> > Index: 2.6.x-xfs-new/fs/xfs/xfs_iget.c
> > ===================================================================
> > --- 2.6.x-xfs-new.orig/fs/xfs/xfs_iget.c 2006-09-14 11:18:52.000000000
> > +1000
> > +++ 2.6.x-xfs-new/fs/xfs/xfs_iget.c 2006-09-14 12:01:04.648209950 +1000
> > @@ -625,7 +617,7 @@ xfs_iput_new(xfs_inode_t *ip,
> > vn_trace_entry(vp, "xfs_iput_new", (inst_t *)__return_address);
> >
> > if ((ip->i_d.di_mode == 0)) {
> > - ASSERT(!(ip->i_flags & XFS_IRECLAIMABLE));
> > + ASSERT(!xfs_iflags_test(ip, XFS_IRECLAIMABLE)) {

+ ASSERT(!xfs_iflags_test(ip, XFS_IRECLAIMABLE));

> > Index: 2.6.x-xfs-new/fs/xfs/xfs_inode.h
> > ===================================================================
> > --- 2.6.x-xfs-new.orig/fs/xfs/xfs_inode.h 2006-09-14 11:18:52.000000000
> > +1000
> > +++ 2.6.x-xfs-new/fs/xfs/xfs_inode.h 2006-09-14 12:32:16.395321563 +1000
> > @@ -305,6 +305,47 @@ typedef struct xfs_inode {
> > #endif
> > } xfs_inode_t;
> >
> > +
> > +/*
> > + * i_flags helper functions
> > + */
> > +static inline void
> > +__xfs_iflags_set(xfs_inode_t *ip, unsigned short flags)
> > +{
> > + ip->i_flags |= flags;
> > +}
> > +
> > +static inline void
> > +xfs_iflags_set(xfs_inode_t *ip, unsigned short flags)
> > +{
> > + spin_lock(&ip->i_flags_lock);
> > + __xfs_iflag_set(ip, flags);

+ __xfs_iflags_set(ip, flags);

David Chinner wrote:
> On Fri, Oct 06, 2006 at 01:26:17PM +1000, David Chinner wrote:
>> I think this is a much better way of fixing the problem, but it needs
>> a little tweaking. Also, it indicates that we can probably revert
>> some of the previous changes made in attempting to fix this bug.
>> I'll put together a new patch with this fix and as much of the
>> other fixes removed as possible and run some tests on it here.
>> It'l be a day or two before I have a tested patch ready....
>
> I've run the attached patch through xfsqa but have not stress tested
> it yet.
>
> Takenori - can you give this a run through your tests to see if
> it passes. I expect any races to trigger the BUG_ON statements
> in xfs_iunpin().
>
> This patch sits on top of iflags locking cleanup I posted here:
>
> http://oss.sgi.com/archives/xfs/2006-10/msg00014.html
>
> Cheers,
>
> Dave.

Best Regards,
--
Takenori Nagano, NEC
t-nagano@ah.jp.nec.com