Re: [PATCH] shmem: factor out remove_indirect_page()

[Hugh Dickins]

On Mon, Apr 11, 2011 at 5:27 AM, Namhyung Kim <namhyung@xxxxxxxxx> wrote:
> Split out some common code in shmem_truncate_range() in order to
> improve readability (hopefully) and to reduce code duplication.
>
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxx>

Thank you for taking the trouble to do this.

However... all that shmem_swp index code is irredeemably unreadable
(my fault, dating from when I converted it to use highmem with kmaps),
and I'd rather leave it untouched until we simply delete it
completely.

I have a patch/set (good for my testing but not yet good for final
submission) which removes all that code, and the need to allocate
shmem_swp index pages (even when CONFIG_SWAP is not set!): instead
saving the swp_entries in the standard pagecache radix_tree for the
file, so no extra allocations are needed at all.

It is possible that my patch/set will not be accepted (extending the
radix_tree in that way may meet some resistance); but I do think
that's the right way forward.

Thanks,
Hugh

> ---
> Âmm/shmem.c | Â 62 ++++++++++++++++++++++++++++++++++-------------------------
> Â1 files changed, 36 insertions(+), 26 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 58da7c150ba6..58ad1159678f 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -531,6 +531,31 @@ static void shmem_free_pages(struct list_head *next)
> Â Â Â Â} while (next);
> Â}
>
> +/**
> + * remove_indirect_page - remove a indirect page from upper layer
> + * @dir: Â Â Â pointer to the indirect page in the upper layer
> + * @page: Â Â Âindirect page to be removed
> + * @needs_lock: Â Â Â Âpointer to spinlock if needed
> + * @free_list: list which the removed page will be inserted into
> + *
> + * This function removes @page from @dir and insert it into @free_list.
> + * The @page still remains after this function but will not be seen by
> + * other tasks. Finally it will be freed via shmem_free_pages().
> + */
> +static void remove_indirect_page(struct page **dir, struct page *page,
> + Â Â Â Â Â Â Â spinlock_t *needs_lock, struct list_head *free_list)
> +{
> + Â Â Â if (needs_lock) {
> + Â Â Â Â Â Â Â spin_lock(needs_lock);
> + Â Â Â Â Â Â Â *dir = NULL;
> + Â Â Â Â Â Â Â spin_unlock(needs_lock);
> + Â Â Â } else {
> + Â Â Â Â Â Â Â *dir = NULL;
> + Â Â Â }
> +
> + Â Â Â list_add(&page->lru, free_list);
> +}
> +
> Âstatic void shmem_truncate_range(struct inode *inode, loff_t start, loff_t end)
> Â{
> Â Â Â Âstruct shmem_inode_info *info = SHMEM_I(inode);
> @@ -582,9 +607,9 @@ static void shmem_truncate_range(struct inode *inode, loff_t start, loff_t end)
>
> Â Â Â Âtopdir = info->i_indirect;
> Â Â Â Âif (topdir && idx <= SHMEM_NR_DIRECT && !punch_hole) {
> - Â Â Â Â Â Â Â info->i_indirect = NULL;
> + Â Â Â Â Â Â Â remove_indirect_page(&info->i_indirect, topdir, NULL,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â&pages_to_free);
> Â Â Â Â Â Â Â Ânr_pages_to_free++;
> - Â Â Â Â Â Â Â list_add(&topdir->lru, &pages_to_free);
> Â Â Â Â}
> Â Â Â Âspin_unlock(&info->lock);
>
> @@ -637,15 +662,10 @@ static void shmem_truncate_range(struct inode *inode, loff_t start, loff_t end)
> Â Â Â Â Â Â Â Â Â Â Â Âdiroff = ((idx - ENTRIES_PER_PAGEPAGE/2) %
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂENTRIES_PER_PAGEPAGE) / ENTRIES_PER_PAGE;
> Â Â Â Â Â Â Â Â Â Â Â Âif (!diroff && !offset && upper_limit >= stage) {
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (needs_lock) {
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â spin_lock(needs_lock);
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â *dir = NULL;
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â spin_unlock(needs_lock);
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â needs_lock = NULL;
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â } else
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â *dir = NULL;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â remove_indirect_page(dir, middir, needs_lock,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â&pages_to_free);
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Ânr_pages_to_free++;
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â list_add(&middir->lru, &pages_to_free);
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â needs_lock = NULL;
> Â Â Â Â Â Â Â Â Â Â Â Â}
> Â Â Â Â Â Â Â Â Â Â Â Âshmem_dir_unmap(dir);
> Â Â Â Â Â Â Â Â Â Â Â Âdir = shmem_dir_map(middir);
> @@ -672,15 +692,10 @@ static void shmem_truncate_range(struct inode *inode, loff_t start, loff_t end)
> Â Â Â Â Â Â Â Â Â Â Â Âif (punch_hole)
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âneeds_lock = &info->lock;
> Â Â Â Â Â Â Â Â Â Â Â Âif (upper_limit >= stage) {
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (needs_lock) {
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â spin_lock(needs_lock);
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â *dir = NULL;
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â spin_unlock(needs_lock);
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â needs_lock = NULL;
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â } else
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â *dir = NULL;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â remove_indirect_page(dir, middir, needs_lock,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â&pages_to_free);
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Ânr_pages_to_free++;
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â list_add(&middir->lru, &pages_to_free);
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â needs_lock = NULL;
> Â Â Â Â Â Â Â Â Â Â Â Â}
> Â Â Â Â Â Â Â Â Â Â Â Âshmem_dir_unmap(dir);
> Â Â Â Â Â Â Â Â Â Â Â Âcond_resched();
> @@ -690,15 +705,10 @@ static void shmem_truncate_range(struct inode *inode, loff_t start, loff_t end)
> Â Â Â Â Â Â Â Âpunch_lock = needs_lock;
> Â Â Â Â Â Â Â Âsubdir = dir[diroff];
> Â Â Â Â Â Â Â Âif (subdir && !offset && upper_limit-idx >= ENTRIES_PER_PAGE) {
> - Â Â Â Â Â Â Â Â Â Â Â if (needs_lock) {
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â spin_lock(needs_lock);
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dir[diroff] = NULL;
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â spin_unlock(needs_lock);
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â punch_lock = NULL;
> - Â Â Â Â Â Â Â Â Â Â Â } else
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dir[diroff] = NULL;
> + Â Â Â Â Â Â Â Â Â Â Â remove_indirect_page(&dir[diroff], subdir, needs_lock,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â&pages_to_free);
> Â Â Â Â Â Â Â Â Â Â Â Ânr_pages_to_free++;
> - Â Â Â Â Â Â Â Â Â Â Â list_add(&subdir->lru, &pages_to_free);
> + Â Â Â Â Â Â Â Â Â Â Â punch_lock = NULL;
> Â Â Â Â Â Â Â Â}
> Â Â Â Â Â Â Â Âif (subdir && page_private(subdir) /* has swap entries */) {
> Â Â Â Â Â Â Â Â Â Â Â Âsize = limit - idx;
> --
> 1.7.4
>
>
¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_