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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
