Couple of code comments

Couple of code comments

[Michael Nishimoto]

Hi,

I've got a couple of code comments.  In xfs_write, there is a
call to bhv_vop_flushinval_pages().  It appears that this function call
is not needed because the call to generic_file_direct_write() done
later in xfs_write does the same work.

generic_file_direct_write -> generic_file_direct_IO ->
(filemap_write_and_wait + invalidate_inode_pages2_range)

bhv_vop_flushinval_pages -> fs_flushinval_pages ->
(filemap_write_and_wait + truncate_inode_pages)

The code isn't exactly the same, but it appears to do the same work.

Also, within xfs_swap_extents() the code fixes up on-disk inode block
counts, but doesn't change the block counts for the Linux inode.

Comments?

    Michael

Re: Couple of code comments

[David Chinner]

On Tue, Jul 24, 2007 at 06:59:53PM -0700, Michael Nishimoto wrote:
> Hi,
> 
> I've got a couple of code comments.  In xfs_write, there is a
> call to bhv_vop_flushinval_pages().  It appears that this function call
> is not needed because the call to generic_file_direct_write() done
> later in xfs_write does the same work.
> 
> generic_file_direct_write -> generic_file_direct_IO ->
> (filemap_write_and_wait + invalidate_inode_pages2_range)
> 
> bhv_vop_flushinval_pages -> fs_flushinval_pages ->
> (filemap_write_and_wait + truncate_inode_pages)

Similar, but seeing we call generic_file_direct_write() without the
i_mutex held and we don't use the generic direct I/O path locking or
flushes (i.e in __blockdev_direct_IO()), we've got to do this flush
somewhere with the i_mutex held....

That being said, xfs_write() is an utter mess and could do
with a major cleanup.

> The code isn't exactly the same, but it appears to do the same work.

Locking is the key difference.

> Also, within xfs_swap_extents() the code fixes up on-disk inode block
> counts, but doesn't change the block counts for the Linux inode.

Patch, please. ;)

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group