| 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
| ||
|
Message-ID: <53F154AE.9010908@lge.com>
Date: Mon, 18 Aug 2014 10:19:42 +0900
From: Gioh Kim <gioh.kim@....com>
To: Andrew Morton <akpm@...ux-foundation.org>
CC: Alexander Viro <viro@...iv.linux.org.uk>,
"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
Peter Zijlstra <peterz@...radead.org>, Jan Kara <jack@...e.cz>,
linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
Theodore Ts'o <tytso@....edu>,
Andreas Dilger <adilger.kernel@...ger.ca>,
linux-ext4@...r.kernel.org, Minchan Kim <minchan@...nel.org>,
Joonsoo Kim <js1304@...il.com>,
이건호 <gunho.lee@....com>
Subject: Re: [PATCH 1/2] fs/buffer.c: allocate buffer cache from non-movable
area
2014-08-15 오전 6:22, Andrew Morton 쓴 글:
> On Thu, 14 Aug 2014 14:15:40 +0900 Gioh Kim <gioh.kim@....com> wrote:
>
>> A buffer cache is allocated from movable area
>> because it is referred for a while and released soon.
>> But some filesystems are taking buffer cache for a long time
>> and it can disturb page migration.
>>
>> A new API should be introduced to allocate buffer cache from
>> non-movable area.
>
> I think the API could and should be more flexible than this.
>
> Rather than making the API be "movable or not movable", let's permit
> callers to specify the gfp_t and leave it at that. That way, if
> someone later wants to allocate a buffer head with, I dunno,
> __GFP_NOTRACK then they can do so.
>
> So the word "movable" shouldn't appear in buffer.c at all, except in a
> single place.
Absolutely I agree with you.
If filesystem developers agree this patch I will send 2nd patch that applies your ideas.
Thank you for your advices.
>
>> --- a/fs/buffer.c
>> +++ b/fs/buffer.c
>> @@ -993,7 +993,7 @@ init_page_buffers(struct page *page, struct block_device *bdev,
>> */
>> static int
>> grow_dev_page(struct block_device *bdev, sector_t block,
>> - pgoff_t index, int size, int sizebits)
>> + pgoff_t index, int size, int sizebits, gfp_t movable_mask)
>
> s/movable_mask/gfp/
I got it.
>
>> {
>> struct inode *inode = bdev->bd_inode;
>> struct page *page;
>> @@ -1003,7 +1003,8 @@ grow_dev_page(struct block_device *bdev, sector_t block,
>> gfp_t gfp_mask;
>>
>> gfp_mask = mapping_gfp_mask(inode->i_mapping) & ~__GFP_FS;
>> - gfp_mask |= __GFP_MOVABLE;
>> + if (movable_mask & __GFP_MOVABLE)
>> + gfp_mask |= __GFP_MOVABLE;
>
> This becomes
>
> gfp_mask |= gfp;
I got it.
>
>> /*
>> * XXX: __getblk_slow() can not really deal with failure and
>> * will endlessly loop on improvised global reclaim. Prefer
>> @@ -1058,7 +1059,8 @@ failed:
>> * that page was dirty, the buffers are set dirty also.
>> */
>> static int
>> -grow_buffers(struct block_device *bdev, sector_t block, int size)
>> +grow_buffers(struct block_device *bdev, sector_t block,
>> + int size, gfp_t movable_mask)
>
> gfp
>
>> {
>> pgoff_t index;
>> int sizebits;
>> @@ -1085,11 +1087,12 @@ grow_buffers(struct block_device *bdev, sector_t block, int size)
>> }
>>
>> /* Create a page with the proper size buffers.. */
>> - return grow_dev_page(bdev, block, index, size, sizebits);
>> + return grow_dev_page(bdev, block, index, size, sizebits, movable_mask);
>> }
>>
>> static struct buffer_head *
>> -__getblk_slow(struct block_device *bdev, sector_t block, int size)
>> +__getblk_slow(struct block_device *bdev, sector_t block,
>> + int size, gfp_t movable_mask)
>
> gfp
>
>> {
>> /* Size must be multiple of hard sectorsize */
>> if (unlikely(size & (bdev_logical_block_size(bdev)-1) ||
>> @@ -1111,7 +1114,7 @@ __getblk_slow(struct block_device *bdev, sector_t block, int size)
>> if (bh)
>> return bh;
>>
>> - ret = grow_buffers(bdev, block, size);
>> + ret = grow_buffers(bdev, block, size, movable_mask);
>
> gfp
>
>> if (ret < 0)
>> return NULL;
>> if (ret == 0)
>> @@ -1385,11 +1388,34 @@ __getblk(struct block_device *bdev, sector_t block, unsigned size)
>>
>> might_sleep();
>> if (bh == NULL)
>> - bh = __getblk_slow(bdev, block, size);
>> + bh = __getblk_slow(bdev, block, size, __GFP_MOVABLE);
>
> Here is the place where buffer.c. mentions "movable".
I got it.
>
>> return bh;
>> }
>> EXPORT_SYMBOL(__getblk);
>>
>> + /*
>> + * __getblk_nonmovable will locate (and, if necessary, create) the buffer_head
>> + * which corresponds to the passed block_device, block and size. The
>> + * returned buffer has its reference count incremented.
>> + *
>> + * The page cache is allocated from non-movable area
>> + * not to prevent page migration.
>> + *
>> + * __getblk()_nonmovable will lock up the machine
>> + * if grow_dev_page's try_to_free_buffers() attempt is failing. FIXME, perhaps?
>> + */
>> +struct buffer_head *
>> +__getblk_nonmovable(struct block_device *bdev, sector_t block, unsigned size)
>> +{
>> + struct buffer_head *bh = __find_get_block(bdev, block, size);
>> +
>> + might_sleep();
>> + if (bh == NULL)
>> + bh = __getblk_slow(bdev, block, size, 0);
>> + return bh;
>> +}
>> +EXPORT_SYMBOL(__getblk_nonmovable);
>
> Suggest this be called __getblk_gfp(bdev, block, size, gfp) and then
> __getblk() be changed to call __getblk_gfp(..., __GFP_MOVABLE).
>
> We could then write a __getblk_nonmovable() which calls __getblk_gfp()
> (a static inlined one-line function) or we can just call
> __getblk_gfp(..., 0) directly from filesystems.
I got it.
>
>> @@ -1423,6 +1450,28 @@ __bread(struct block_device *bdev, sector_t block, unsigned size)
>> }
>> EXPORT_SYMBOL(__bread);
>>
>> +/**
>> + * __bread_nonmovable() - reads a specified block and returns the bh
>> + * @bdev: the block_device to read from
>> + * @block: number of block
>> + * @size: size (in bytes) to read
>> + *
>> + * Reads a specified block, and returns buffer head that contains it.
>> + * The page cache is allocated from non-movable area
>> + * not to prevent page migration.
>> + * It returns NULL if the block was unreadable.
>> + */
>> +struct buffer_head *
>> +__bread_nonmovable(struct block_device *bdev, sector_t block, unsigned size)
>> +{
>> + struct buffer_head *bh = __getblk_slow(bdev, block, size, 0);
>> +
>> + if (likely(bh) && !buffer_uptodate(bh))
>> + bh = __bread_slow(bh);
>> + return bh;
>> +}
>> +EXPORT_SYMBOL(__bread_nonmovable);
>
> Treat this in the same fashion as __getblk_nonmovable().
I got it.
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists