by Sergey S. Kostyliov

[permalink] [raw]

Subject: Re: 2.4.22pre6aa1

Hi Hugh and Andrew,

On Saturday 16 August 2003 18:00, Hugh Dickins wrote:
> On Sat, 16 Aug 2003, Hugh Dickins wrote:
> > Or, as in the patch Sergey is currently testing below, shmem_truncate
> > must be prepared to truncate_inode_pages again. That's the approach
> > I originally implemented in 2.5, but I grew disgusted with it every
> > time I thought of partial truncation trundling twice through
> > truncate_inode_pages (it can easily be avoided when nrpages == 0,
> > but that's unlikely in partial truncation).
> >
> > So VM_PAGEIN flag stuff to restrict it to when it might be necessary;
> > extended to cover other races when reading the page at the same time
> > as truncating (though I think generic_file_read has a window of this
> > kind that we've never worried about). I expect to split the patch
> > into several before sending Marcelo and Andrew.
>
> And here is the patch I claimed to be below.
> If you apply it to anything other than 2.4.22-pre6aa1,
> please be careful to check that it has applied correctly.
> Originally I made a patch against 2.4.22-pre6, and then applied to
> 2.4.22-pre6aa1: but I have never seen patch make such a mess of it!

I just want to confirm that I wasn't able to repeat this problem
with the patch below (applied to 2.4.22-pre7aa1) after more than
3 days of testing. I'll inform you if any issues will arise.
Thank you!

>
> Hugh
>
> --- 2.4.22-pre6aa1/mm/shmem.c Thu Jul 31 15:23:58 2003
> +++ linux/mm/shmem.c Mon Aug 11 21:00:55 2003
> @@ -92,6 +92,9 @@
>
> #define VM_ACCT(size) (PAGE_CACHE_ALIGN(size) >> PAGE_SHIFT)
>
> +/* info->flags needs a VM_flag to handle pagein/truncate race efficiently
> */ +#define VM_PAGEIN VM_READ
> +
> /* Pretend that each entry is of this size in directory's i_size */
> #define BOGO_DIRENT_SIZE 20
>
> @@ -435,6 +438,18 @@
>
> BUG_ON(info->swapped > info->next_index);
> spin_unlock(&info->lock);
> +
> + if (inode->i_mapping->nrpages && (info->flags & VM_PAGEIN)) {
> + /*
> + * Call truncate_inode_pages again: racing shmem_unuse_inode
> + * may have swizzled a page in from swap since vmtruncate or
> + * generic_delete_inode did it, before we lowered next_index.
> + * Also, though shmem_getpage checks i_size before adding to
> + * cache, no recheck after: so fix the narrow window there too.
> + */
> + truncate_inode_pages(inode->i_mapping, inode->i_size);
> + }
> +
> if (freed)
> shmem_free_blocks(inode, freed);
> }
> @@ -459,6 +474,19 @@
> attr->ia_size>>PAGE_CACHE_SHIFT,
> &page, SGP_READ);
> }
> + /*
> + * Reset VM_PAGEIN flag so that shmem_truncate can
> + * detect if any pages might have been added to cache
> + * after truncate_inode_pages. But we needn't bother
> + * if it's being fully truncated to zero-length: the
> + * nrpages check is efficient enough in that case.
> + */
> + if (attr->ia_size) {
> + struct shmem_inode_info *info = SHMEM_I(inode);
> + spin_lock(&info->lock);
> + info->flags &= ~VM_PAGEIN;
> + spin_unlock(&info->lock);
> + }
> }
> }
>
> @@ -511,7 +539,6 @@
> struct address_space *mapping;
> swp_entry_t *ptr;
> unsigned long idx;
> - unsigned long limit;
> int offset;
>
> idx = 0;
> @@ -543,13 +570,9 @@
> inode = info->inode;
> mapping = inode->i_mapping;
> delete_from_swap_cache(page);
> -
> - /* Racing against delete or truncate? Must leave out of page cache */
> - limit = (inode->i_state & I_FREEING)? 0:
> - (inode->i_size + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
> -
> - if (idx >= limit || add_to_page_cache_unique(page,
> - mapping, idx, page_hash(mapping, idx)) == 0) {
> + if (add_to_page_cache_unique(page,
> + mapping, idx, page_hash(mapping, idx)) == 0) {
> + info->flags |= VM_PAGEIN;
> ptr[offset].val = 0;
> info->swapped--;
> } else if (add_to_swap_cache(page, entry) != 0)
> @@ -634,6 +657,7 @@
> * Add page back to page cache, unref swap, try again.
> */
> add_to_page_cache_locked(page, mapping, index);
> + info->flags |= VM_PAGEIN;
> spin_unlock(&info->lock);
> swap_free(swap);
> goto getswap;
> @@ -809,6 +833,7 @@
> swap_free(swap);
> } else if (add_to_page_cache_unique(swappage,
> mapping, idx, page_hash(mapping, idx)) == 0) {
> + info->flags |= VM_PAGEIN;
> entry->val = 0;
> info->swapped--;
> spin_unlock(&info->lock);
> @@ -868,6 +893,7 @@
> goto failed;
> goto repeat;
> }
> + info->flags |= VM_PAGEIN;
> }
>
> spin_unlock(&info->lock);

--
Best regards,
Sergey S. Kostyliov <[email protected]>
Public PGP key: http://sysadminday.org.ru/rathamahata.asc