On Wed, 2010-12-01 at 19:34 -0800, Hugh Dickins wrote: > On Wed, 1 Dec 2010, Trond Myklebust wrote: > > > > --- a/mm/truncate.c > > +++ b/mm/truncate.c > > @@ -108,6 +108,10 @@ truncate_complete_page(struct address_space *mapping, struct page *page) > > clear_page_mlock(page); > > remove_from_page_cache(page); > > ClearPageMappedToDisk(page); > > + > > + if (mapping->a_ops->freepage) > > + mapping->a_ops->freepage(page); > > + > > page_cache_release(page); /* pagecache ref */ > > return 0; > > } > > I think Linus recommended that one be done in remove_from_page_cache() > to catch all instances: did that get overruled later for some reason? I'm fine with doing that as long as everyone is happy that there is no chance of races. I was a bit nervous given the discussion that ensued from the vmscan case. > > --- a/mm/vmscan.c > > +++ b/mm/vmscan.c > > @@ -454,6 +454,7 @@ static int __remove_mapping(struct address_space *mapping, struct page *page) > > BUG_ON(!PageLocked(page)); > > BUG_ON(mapping != page_mapping(page)); > > > > + preempt_disable(); > > spin_lock_irq(&mapping->tree_lock); > > /* > > * The non racy check for a busy page. > > @@ -492,10 +493,19 @@ static int __remove_mapping(struct address_space *mapping, struct page *page) > > swp_entry_t swap = { .val = page_private(page) }; > > __delete_from_swap_cache(page); > > spin_unlock_irq(&mapping->tree_lock); > > + preempt_enable(); > > swapcache_free(swap, page); > > } else { > > + void (*freepage)(struct page *); > > + > > + freepage = mapping->a_ops->freepage; > > + > > __remove_from_page_cache(page); > > spin_unlock_irq(&mapping->tree_lock); > > + if (freepage != NULL) > > + freepage(page); > > + preempt_enable(); > > + > > mem_cgroup_uncharge_cache_page(page); > > } > > > > @@ -503,6 +513,7 @@ static int __remove_mapping(struct address_space *mapping, struct page *page) > > > > cannot_free: > > spin_unlock_irq(&mapping->tree_lock); > > + preempt_enable(); > > return 0; > > } > > I took his "stop_machine()" explanation ("an idle period for everything. > And just a preemption reschedule isn't enough for that") to imply that > there's no need for your preempt_disable/preempt_enable there: they > don't add anything to the module unload case, and they don't help the > spin_unlock_irq issue (and you're already being rightly careful to note > freepage in advance). > > But maybe I misunderstood. Again, I was being cautious (I freely admit to not having studied stop_machine()). If nobody disagrees with your interpretation, then I'm very happy to drop the preempt disable/enable crud. Cheers Trond -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@xxxxxxxxxx www.netapp.com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html