linux-kernel - Re: [PATCH 1/3] writeback: Creating /sys/kernel/mm/writeback/writeback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AANLkTimawKkDNpaVrfQtfxPah3QduodLK2njWLTMOhMl@mail.gmail.com>
Date:	Sat, 19 Jun 2010 10:44:46 -0700
From:	Michael Rubin <mrubin@...gle.com>
To:	Christoph Hellwig <hch@....de>
Cc:	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	linux-mm@...ck.org, jack@...e.cz, akpm@...ux-foundation.org,
	david@...morbit.com, axboe@...nel.dk
Subject: Re: [PATCH 1/3] writeback: Creating /sys/kernel/mm/writeback/writeback

Thanks for looking at this.

On Sat, Jun 19, 2010 at 3:44 AM, Christoph Hellwig <hch@....de> wrote:
> I'm fine with exposting this. but the interface is rather awkward.
> These kinds of multiple value per file interface require addition
> parsing and are a pain to extend.  Please do something like
>
> /proc/sys/vm/writeback/
>
>                        pages_dirtied
>                        pages_cleaned
>                        dirty_threshold
>                        background_threshold
>
> where you can just read the value from the file.

Cool. This is kind of funny. In the google tree I implemented this in
the same multi-file-one-value-in-file manner. The debate on one file
for all vs that style was heated. And I changed it before sending
upstream. I really don't care either way. So I will just change the
patch and move the values to that location

Do you mind explaining why something would go in /proc/ vs /sys? I
thought the idea was to not put things in /proc anymore.

>> diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
>> index c920164..84b0181 100644
>> --- a/fs/nilfs2/segment.c
>> +++ b/fs/nilfs2/segment.c
>> @@ -1598,8 +1598,10 @@ nilfs_copy_replace_page_buffers(struct page *page, struct list_head *out)
>>       } while (bh = bh->b_this_page, bh2 = bh2->b_this_page, bh != head);
>>       kunmap_atomic(kaddr, KM_USER0);
>>
>> -     if (!TestSetPageWriteback(clone_page))
>> +     if (!TestSetPageWriteback(clone_page)) {
>>               inc_zone_page_state(clone_page, NR_WRITEBACK);
>> +             inc_zone_page_state(clone_page, NR_PAGES_ENTERED_WRITEBACK);
>> +     }
>>       unlock_page(clone_page);
>
> I'm not very happy about having this opencoded in a filesystem.

I wasn't excited about this section either. What does opencoded mean?
Do you mean it should not be exposed to specific fs code?

mrubin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/