lkml.org 
[lkml]   [2010]   [May]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/3] Support generic I/O requests
Hi Minchan,

On 05/31/2010 09:11 PM, Minchan Kim wrote:
>
> On Mon, 2010-05-24 at 19:48 +0530, Nitin Gupta wrote:
<snip>
>>
>> - Scalability: There is only one (per-device) de/compression
>> buffer stats. This can lead to significant contention, especially
>> when used for generic (non-swap) purposes.
>
> Later, we could enhance it by per-cpu counter.
>

Yes, currently we effectively use just 1 core due to a single
compression buffer. So, per-cpu buffers and counters should
really help.

>> -static int handle_zero_page(struct bio *bio)
>> +static void handle_zero_page(struct page *page, u32 index)
>
> It doesn't use index.
>

Ok, I will remove this argument.

>>
>> - /* Page is stored uncompressed since it's incompressible */
>> - if (unlikely(rzs_test_flag(rzs, index, RZS_UNCOMPRESSED)))
>> - return handle_uncompressed_page(rzs, bio);
>> + /* Requested page is not present in compressed area */
>> + if (unlikely(!rzs->table[index].page)) {
>> + pr_debug("Read before write on swap device: "
> ^^^^
> It's not ramzswap any more. It is zram. :)
>

I will correct this :)


>> + /*
>> + * Page is incompressible. Store it as-is (uncompressed)
>> + * since we do not want to return too many swap write
>> + * errors which has side effect of hanging the system.
>> + */
>> + if (unlikely(clen > max_zpage_size)) {
>> + clen = PAGE_SIZE;
>> + page_store = alloc_page(GFP_NOIO | __GFP_HIGHMEM);
>> + if (unlikely(!page_store)) {
>> + mutex_unlock(&rzs->lock);
>
> What's purpose of rzs->lock?
> Please, Document it.
>


Ok. I intentionally deferred much of the documentation for later patches
to reduce "side noise" for these patches. Anyways, I will document this
lock in "v2" patches.

> And please, take care of "swap" mentioned in comments.
>

Old habits die hard :) I will clean them up.

> I think code doesn't have big problem.
> But my feel is we can clean up some portion of codes. But it's not a big
> deal. It doesn't have any problem to work.
> So I want to add zram into linux-next, too.
>

I will be sending patch series with code commentary, cleanups and minor
fixes once this much enters linux-next.

Thanks for the review.
Nitin


\
 
 \ /
  Last update: 2010-05-31 21:25    [W:0.061 / U:1.348 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site