Re: [PATCH 4/9] exofs: address_space_operations

[Boaz Harrosh <bharrosh@panasas.com>]

Andrew Morton wrote:
>> +
>> +	kaddr = page_address(page);
>> +
>> +	req = prepare_osd_write(sbi->s_dev, sbi->s_pid,
>> +			      inode->i_ino + EXOFS_OBJ_OFF, len, start, 0,
>> +			      kaddr);
> 
> Does prepare_osd_write() modify the memory at *kaddr?  If so, does it
> do the needed flush_dcache_page()?
> 

kaddr is not modified by CPU. This is just a very BAD API left from the old
osd-initiator days.The address is used for preparing a BIO and submitted
to HW later. I will change all these places to receive a page* directly.
(It was meant to be changed in the future where I want to support read/write 
 page* array).

<snip>
>> +	} else if (ret == -EFAULT) {
>> +		char *kaddr;
>> +
>> +		/* In this case we were trying to read something that wasn't on
>> +		 * disk yet - return a page full of zeroes.  This should be OK,
>> +		 * because the object should be empty (if there was a write
>> +		 * before this read, the read would be waiting with the page
>> +		 * locked */
>> +		kaddr = page_address(page);
>> +		memset(kaddr, 0, PAGE_CACHE_SIZE);
> 
> There is I think a missing flsh_dcache_page() here.  Use of the
> (somewhat misnamed) zero_user() would be an appropriate fix and
> cleanup.
> 

What happened here is that the HW actually never touched the page in question,
and it is returned to CPU, do I need to flsh_dcache_page anyway?

But this is not relevant since I will use zero_user() as you suggested.
Should I use clear_highpage as this is a clear of a full page?

<snip>
>> +
>> +	/* this will be out of bounds, or doesn't exist yet */
>> +	if ((page->index >= end_index + 1) || !ObjCreated(oi) || !amount
>> +	    /*|| (i_start >= oi->i_commit_size)*/) {
>> +		kaddr = kmap_atomic(page, KM_USER0);
>> +		memset(kaddr, 0, PAGE_CACHE_SIZE);
>> +		flush_dcache_page(page);
>> +		kunmap_atomic(page, KM_USER0);
> 
> There's a flush_dcache_page() ;)
> 
> Could use clear_highpage() here.
> 

Thanks, sounds much better.

>> +		SetPageUptodate(page);
>> +		if (PageError(page))
>> +			ClearPageError(page);
>> +		if (is_async_unlock)
>> +			unlock_page(page);
>> +		goto out;
>> +	}
>> +
>> +	if (amount != PAGE_CACHE_SIZE) {
>> +		kaddr = kmap_atomic(page, KM_USER0);
>> +		memset(kaddr + amount, 0, PAGE_CACHE_SIZE - amount);
>> +		flush_dcache_page(page);
>> +		kunmap_atomic(page, KM_USER0);
> 
> Use zero_user()?
> 

Will change

Thanks
Boaz