| 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: <20130111201211.GA20981@quack.suse.cz>
Date: Fri, 11 Jan 2013 21:12:11 +0100
From: Jan Kara <jack@...e.cz>
To: Miao Xie <miaox@...fujitsu.com>
Cc: Linux Fsdevel <linux-fsdevel@...r.kernel.org>,
Linux Btrfs <linux-btrfs@...r.kernel.org>,
Linux Ext4 <linux-ext4@...r.kernel.org>,
Linux Kernel <linux-kernel@...r.kernel.org>, dsterba@...e.cz,
Christoph Hellwig <hch@...radead.org>,
Kamal Mostafa <kamal@...onical.com>,
Al Viro <viro@...IV.linux.org.uk>,
Wu Fengguang <fengguang.wu@...el.com>
Subject: Re: [PATCH V2] vfs: re-implement
writeback_inodes_sb(_nr)_if_idle() and rename them
On Thu 10-01-13 13:47:57, Miao Xie wrote:
> writeback_inodes_sb(_nr)_if_idle() is re-implemented by replacing down_read()
> with down_read_trylock() because
> - If ->s_umount is write locked, then the sb is not idle. That is
> writeback_inodes_sb(_nr)_if_idle() needn't wait for the lock.
> - writeback_inodes_sb(_nr)_if_idle() grabs s_umount lock when it want to start
> writeback, it may bring us deadlock problem when doing umount. In order to
> fix the problem, ext4 and btrfs implemented their own writeback functions
> instead of writeback_inodes_sb(_nr)_if_idle(), but it introduced the redundant
> code, it is better to implement a new writeback_inodes_sb(_nr)_if_idle().
>
> The name of these two functions is cumbersome, so rename them to
> try_to_writeback_inodes_sb(_nr).
>
> This idea came from Christoph Hellwig.
> Some code is from the patch of Kamal Mostafa.
The patch looks good. Thanks! You can add:
Reviewed-by: Jan Kara <jack@...e.cz>
BTW, since changes to filesystems are minimal, maybe Fengguang can take
the patch through his writeback tree? Fengguang?
Honza
>
> Signed-off-by: Miao Xie <miaox@...fujitsu.com>
> ---
> Changelog v1 -> v2:
> - do not remove EXPORT_SYMBOL of writeback_inodes_sb_br()
> ---
> fs/btrfs/extent-tree.c | 20 +++-----------------
> fs/ext4/inode.c | 8 ++------
> fs/fs-writeback.c | 44 ++++++++++++++++++++------------------------
> include/linux/writeback.h | 6 +++---
> 4 files changed, 28 insertions(+), 50 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 521e9d4..f31abb1 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -3689,20 +3689,6 @@ static int can_overcommit(struct btrfs_root *root,
> return 0;
> }
>
> -static int writeback_inodes_sb_nr_if_idle_safe(struct super_block *sb,
> - unsigned long nr_pages,
> - enum wb_reason reason)
> -{
> - if (!writeback_in_progress(sb->s_bdi) &&
> - down_read_trylock(&sb->s_umount)) {
> - writeback_inodes_sb_nr(sb, nr_pages, reason);
> - up_read(&sb->s_umount);
> - return 1;
> - }
> -
> - return 0;
> -}
> -
> /*
> * shrink metadata reservation for delalloc
> */
> @@ -3735,9 +3721,9 @@ static void shrink_delalloc(struct btrfs_root *root, u64 to_reclaim, u64 orig,
> while (delalloc_bytes && loops < 3) {
> max_reclaim = min(delalloc_bytes, to_reclaim);
> nr_pages = max_reclaim >> PAGE_CACHE_SHIFT;
> - writeback_inodes_sb_nr_if_idle_safe(root->fs_info->sb,
> - nr_pages,
> - WB_REASON_FS_FREE_SPACE);
> + try_to_writeback_inodes_sb_nr(root->fs_info->sb,
> + nr_pages,
> + WB_REASON_FS_FREE_SPACE);
>
> /*
> * We need to wait for the async pages to actually start before
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index cbfe13b..5f6eef7 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2512,12 +2512,8 @@ static int ext4_nonda_switch(struct super_block *sb)
> /*
> * Start pushing delalloc when 1/2 of free blocks are dirty.
> */
> - if (dirty_blocks && (free_blocks < 2 * dirty_blocks) &&
> - !writeback_in_progress(sb->s_bdi) &&
> - down_read_trylock(&sb->s_umount)) {
> - writeback_inodes_sb(sb, WB_REASON_FS_FREE_SPACE);
> - up_read(&sb->s_umount);
> - }
> + if (dirty_blocks && (free_blocks < 2 * dirty_blocks))
> + try_to_writeback_inodes_sb(sb, WB_REASON_FS_FREE_SPACE);
>
> if (2 * free_blocks < 3 * dirty_blocks ||
> free_blocks < (dirty_blocks + EXT4_FREECLUSTERS_WATERMARK)) {
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 310972b..ad3cc46 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -1332,47 +1332,43 @@ void writeback_inodes_sb(struct super_block *sb, enum wb_reason reason)
> EXPORT_SYMBOL(writeback_inodes_sb);
>
> /**
> - * writeback_inodes_sb_if_idle - start writeback if none underway
> + * try_to_writeback_inodes_sb_nr - try to start writeback if none underway
> * @sb: the superblock
> - * @reason: reason why some writeback work was initiated
> + * @nr: the number of pages to write
> + * @reason: the reason of writeback
> *
> - * Invoke writeback_inodes_sb if no writeback is currently underway.
> + * Invoke writeback_inodes_sb_nr if no writeback is currently underway.
> * Returns 1 if writeback was started, 0 if not.
> */
> -int writeback_inodes_sb_if_idle(struct super_block *sb, enum wb_reason reason)
> +int try_to_writeback_inodes_sb_nr(struct super_block *sb,
> + unsigned long nr,
> + enum wb_reason reason)
> {
> - if (!writeback_in_progress(sb->s_bdi)) {
> - down_read(&sb->s_umount);
> - writeback_inodes_sb(sb, reason);
> - up_read(&sb->s_umount);
> + if (writeback_in_progress(sb->s_bdi))
> return 1;
> - } else
> +
> + if (!down_read_trylock(&sb->s_umount))
> return 0;
> +
> + writeback_inodes_sb_nr(sb, nr, reason);
> + up_read(&sb->s_umount);
> + return 1;
> }
> -EXPORT_SYMBOL(writeback_inodes_sb_if_idle);
> +EXPORT_SYMBOL(try_to_writeback_inodes_sb_nr);
>
> /**
> - * writeback_inodes_sb_nr_if_idle - start writeback if none underway
> + * try_to_writeback_inodes_sb - try to start writeback if none underway
> * @sb: the superblock
> - * @nr: the number of pages to write
> * @reason: reason why some writeback work was initiated
> *
> - * Invoke writeback_inodes_sb if no writeback is currently underway.
> + * Implement by try_to_writeback_inodes_sb_nr()
> * Returns 1 if writeback was started, 0 if not.
> */
> -int writeback_inodes_sb_nr_if_idle(struct super_block *sb,
> - unsigned long nr,
> - enum wb_reason reason)
> +int try_to_writeback_inodes_sb(struct super_block *sb, enum wb_reason reason)
> {
> - if (!writeback_in_progress(sb->s_bdi)) {
> - down_read(&sb->s_umount);
> - writeback_inodes_sb_nr(sb, nr, reason);
> - up_read(&sb->s_umount);
> - return 1;
> - } else
> - return 0;
> + return try_to_writeback_inodes_sb_nr(sb, get_nr_dirty_pages(), reason);
> }
> -EXPORT_SYMBOL(writeback_inodes_sb_nr_if_idle);
> +EXPORT_SYMBOL(try_to_writeback_inodes_sb);
>
> /**
> * sync_inodes_sb - sync sb inode pages
> diff --git a/include/linux/writeback.h b/include/linux/writeback.h
> index b82a83a..9a9367c 100644
> --- a/include/linux/writeback.h
> +++ b/include/linux/writeback.h
> @@ -87,9 +87,9 @@ int inode_wait(void *);
> void writeback_inodes_sb(struct super_block *, enum wb_reason reason);
> void writeback_inodes_sb_nr(struct super_block *, unsigned long nr,
> enum wb_reason reason);
> -int writeback_inodes_sb_if_idle(struct super_block *, enum wb_reason reason);
> -int writeback_inodes_sb_nr_if_idle(struct super_block *, unsigned long nr,
> - enum wb_reason reason);
> +int try_to_writeback_inodes_sb(struct super_block *, enum wb_reason reason);
> +int try_to_writeback_inodes_sb_nr(struct super_block *, unsigned long nr,
> + enum wb_reason reason);
> void sync_inodes_sb(struct super_block *);
> long writeback_inodes_wb(struct bdi_writeback *wb, long nr_pages,
> enum wb_reason reason);
> --
> 1.7.11.7
>
> --
> 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/
--
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
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