[Ocfs2-devel] [PATCH 1/4] ocfs2: Add struct file to ocfs2_refcount_cow.

[Tao Ma <tao.ma@oracle.com>]

Hi Joel,

On 06/30/2010 07:14 AM, Joel Becker wrote:
> On Tue, Jun 29, 2010 at 04:35:38PM +0800, Tao Ma wrote:
>> Add a new parameter 'struct file *' to ocfs2_refcount_cow
>> so that we can add readahead support later.
>>
>> Signed-off-by: Tao Ma<tao.ma@oracle.com>
>> ---
>>   fs/ocfs2/aops.c         |    2 +-
>>   fs/ocfs2/file.c         |   13 ++++++++-----
>>   fs/ocfs2/refcounttree.c |    4 +++-
>>   fs/ocfs2/refcounttree.h |    3 ++-
>>   4 files changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
>> index 4dfaa6e..5f94923 100644
>> --- a/fs/ocfs2/aops.c
>> +++ b/fs/ocfs2/aops.c
>> @@ -1691,7 +1691,7 @@ int ocfs2_write_begin_nolock(struct address_space *mapping,
>>   		mlog_errno(ret);
>>   		goto out;
>>   	} else if (ret == 1) {
>> -		ret = ocfs2_refcount_cow(inode, di_bh,
>> +		ret = ocfs2_refcount_cow(inode, NULL, di_bh,
>>   					 wc->w_cpos, wc->w_clen, UINT_MAX);
>
> 	You should be replacing the inode parameter with the file
> parameter.  You can always get back to the inode from the filp.  When I
> first saw this, I had to read through your entire series to figure out
> if a NULL filp was valid.  It's not.  In the end, you change
> write_begin_nolock() to take (inode, filp), which is pointless, because
> write_begin() always gets a valid filp.  So does page_mkwrite().
> 	So in your first patch of the series, change
> ocfs2_write_begin_nolock() and __ocfs2_page_mkwrite() to take the filp
> instead of the inode.  Then in your later patches you can always expect
> the filp to be valid.
oh, sorry joel. There does exist one place that we don't have filp. It 
is in ocfs2_orphan_for_truncate, we have to CoW the cluster containing 
i_size and pass NULL as filp.

Regards,
Tao