Re: 2.6.2-rc2 nfsd+xfs spins in i_size_read()

[Steve Lord <lord@xfs.org>]

Miquel van Smoorenburg wrote:
> On Sat, 31 Jan 2004 02:38:51, Andrew Morton wrote:
> 
>>Miquel van Smoorenburg <miquels@cistron.nl> wrote:
>>
>>>But the XFS problem appears to be vn_revalidate which calls i_size_write()
>>>without holding i_sem:
>>
>>There's your bug.
> 
> 
> Okay. It seems that XFS uses its own locking with xfs_ilock() etc,
> so I am not sure if this should be fixed by using down(&inode->i_sem)
> or by using xfs_ilock().
> 
> Perhaps xfs_ilock() should also get the inode->i_sem semaphore
> in the XFS_ILOCK_EXCL case ?
> 
> Also, there are one or two more places that call i_size_write()
> that should be looked at I guess.
> 

Actually it gets more messy than this. i_size_write is called by
different spots by xfs without the i_sem held. The call in
generic_file_aio_write_nolock() is made without the lock for O_DIRECT 
writes, XFS allows multiple parallel writes for O_DIRECT
writes.

The vn_revalidate call is called out of linvfs_setattr,
which is called with the i_sem held, it is also potentially called out
of linvfs_getattr, although since the i_size is always maintained
as it is changed, this call should not actually be updating the size.
Possibly changing the code in vn_revalidate to do this:

	if (i_size_read(inode) < va.va_size))
		i_size_write(inode, va.va_size);

Would be a good starting point, I suspect those calls from the nfs
revalidate call are not really going to change the inode size. My
guess is this will make your problem go away.

Probably some larger code restructure is needed so that revalidate
knows if the i_sem is held or not at this point.

The O_DIRECT write case is the hard one. In XFS's internal view of
the world, the inode size is maintained via the XFS_ILOCK, but we
only hold that across metadata manipulation within the fs code,
not across I/O such as a call to generic_file_aio_write_nolock.
Right now the only way I see of dealing with that is to make
writes which we know will extent the file hold the i_sem for
the duration in the O_DIRECT case.

Steve