Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
Hi, On Sat, Mar 24, 2001 at 10:05:18PM -0300, Rik van Riel wrote: > On Sun, 25 Mar 2001, Stephen C. Tweedie wrote: > > > Rik, do you think it is really necessary to take the page lock and > > release it inside lookup_swap_cache? I may be overlooking something, > > but I can't see the benefit of it --- > > I don't think we need to do this, except to protect us from > using a page which isn't up-to-date yet and locked because > of disk IO. But it doesn't --- page_launder can try to lock the page after it checks the refcount, without taking any locks which protect us against running lookup_swap_cache in parallel. If we get our reference after page_launder checks the count, we can find the page getting locked out from underneath our feet. > Reclaim_page() takes the pagecache_lock before trying to > free anything, so there's no reason to lock against that. Exactly. We're not in danger of _losing_ the page, because reclaim_page is locked more aggressively than page_launder. We still risk having the page locked against us after lookup_swap_cache does its own UnlockPage. So, if lookup_swap_cache doesn't actually ensure that the page is unlocked, are there any callers which implicitly rely on lookup_swap_cache() doing a wait_on_page? --Stephen - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
Hi, On Sat, Mar 24, 2001 at 10:05:18PM -0300, Rik van Riel wrote: On Sun, 25 Mar 2001, Stephen C. Tweedie wrote: Rik, do you think it is really necessary to take the page lock and release it inside lookup_swap_cache? I may be overlooking something, but I can't see the benefit of it --- I don't think we need to do this, except to protect us from using a page which isn't up-to-date yet and locked because of disk IO. But it doesn't --- page_launder can try to lock the page after it checks the refcount, without taking any locks which protect us against running lookup_swap_cache in parallel. If we get our reference after page_launder checks the count, we can find the page getting locked out from underneath our feet. Reclaim_page() takes the pagecache_lock before trying to free anything, so there's no reason to lock against that. Exactly. We're not in danger of _losing_ the page, because reclaim_page is locked more aggressively than page_launder. We still risk having the page locked against us after lookup_swap_cache does its own UnlockPage. So, if lookup_swap_cache doesn't actually ensure that the page is unlocked, are there any callers which implicitly rely on lookup_swap_cache() doing a wait_on_page? --Stephen - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
On Sun, 25 Mar 2001, Stephen C. Tweedie wrote: > Rik, do you think it is really necessary to take the page lock and > release it inside lookup_swap_cache? I may be overlooking something, > but I can't see the benefit of it --- I don't think we need to do this, except to protect us from using a page which isn't up-to-date yet and locked because of disk IO. Reclaim_page() takes the pagecache_lock before trying to free anything, so there's no reason to lock against that. regards, Rik -- Virtual memory is like a game you can't win; However, without VM there's truly nothing to lose... http://www.surriel.com/ http://www.conectiva.com/ http://distro.conectiva.com.br/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
Hi, On Fri, Mar 23, 2001 at 11:58:50AM -0800, Linus Torvalds wrote: > Ehh.. Sleeping with the spin-lock held? Sounds like a truly bad idea. Uggh --- the shmem code already does, see: shmem_truncate->shmem_truncate_part->shmem_free_swp-> lookup_swap_cache->find_lock_page It looks messy: lookup_swap_cache seems to be abusing the page lock gratuitously, but there are probably callers of it which rely on the assumption that it performs an implicit wait_on_page(). Rik, do you think it is really necessary to take the page lock and release it inside lookup_swap_cache? I may be overlooking something, but I can't see the benefit of it --- we can still race against page_launder, so the page may still get locked behind our backs after we get the reference from lookup_swap_cache (page_launder explicitly avoids taking the pagecache hash spinlock which might avoid this particular race). --Stephen - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
Hi, On Fri, Mar 23, 2001 at 11:58:50AM -0800, Linus Torvalds wrote: Ehh.. Sleeping with the spin-lock held? Sounds like a truly bad idea. Uggh --- the shmem code already does, see: shmem_truncate-shmem_truncate_part-shmem_free_swp- lookup_swap_cache-find_lock_page It looks messy: lookup_swap_cache seems to be abusing the page lock gratuitously, but there are probably callers of it which rely on the assumption that it performs an implicit wait_on_page(). Rik, do you think it is really necessary to take the page lock and release it inside lookup_swap_cache? I may be overlooking something, but I can't see the benefit of it --- we can still race against page_launder, so the page may still get locked behind our backs after we get the reference from lookup_swap_cache (page_launder explicitly avoids taking the pagecache hash spinlock which might avoid this particular race). --Stephen - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
On Sun, 25 Mar 2001, Stephen C. Tweedie wrote: Rik, do you think it is really necessary to take the page lock and release it inside lookup_swap_cache? I may be overlooking something, but I can't see the benefit of it --- I don't think we need to do this, except to protect us from using a page which isn't up-to-date yet and locked because of disk IO. Reclaim_page() takes the pagecache_lock before trying to free anything, so there's no reason to lock against that. regards, Rik -- Virtual memory is like a game you can't win; However, without VM there's truly nothing to lose... http://www.surriel.com/ http://www.conectiva.com/ http://distro.conectiva.com.br/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
On Fri, 23 Mar 2001, Alan Cox wrote: > > __find_get_page has I think a misleading comment ? Ehh.. I only said the _naming_ makes sense. [ Wild hand-waving ] I suspect that what happened was that we split off the functions (one to just get the page, one to lock it), and the comment that was associated with the original "find_page()" never got removed, and just happens to sit above one of the helper functions now - the one that didn't lock. I'll fix the comment. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
> If you don't want to sleep, you need to use one of the wrappers for > "__find_page_nolock()". Something like "find_get_page()", which only > "gets" the page. * a rather lightweight function, finding and getting a reference to a * hashed page atomically, waiting for it if it's locked. __find_get_page has I think a misleading comment ? > The naming actually does make sense in this area. Yep - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
Alan Cox writes: > Umm find_lock_page doesnt sleep does it ? It does lock_page, which sleeps to get the lock if necessary. Later, David S. Miller [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
> > > + page = find_lock_page(mapping, idx); > > > > > > Ehh.. Sleeping with the spin-lock held? Sounds like a truly bad idea. > > > > Umm find_lock_page doesnt sleep does it ? > > It certainly does. find_lock_page() -> __find_lock_page() -> lock_page() -> > -> __lock_page() -> schedule(). Ok I missed the lock page one. Yep. Alan - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
On Fri, 23 Mar 2001, Alan Cox wrote: > > On Fri, 23 Mar 2001, Stephen C. Tweedie wrote: > > > > > > The patch below is for two races in sysV shared memory. > > > > + spin_lock (>lock); > > + > > + /* The shmem_swp_entry() call may have blocked, and > > +* shmem_writepage may have been moving a page between the page > > +* cache and swap cache. We need to recheck the page cache > > +* under the protection of the info->lock spinlock. */ > > + > > + page = find_lock_page(mapping, idx); > > > > Ehh.. Sleeping with the spin-lock held? Sounds like a truly bad idea. > > Umm find_lock_page doesnt sleep does it ? It certainly does. find_lock_page() -> __find_lock_page() -> lock_page() -> -> __lock_page() -> schedule(). - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
On Fri, 23 Mar 2001, Alan Cox wrote: > > > > + spin_lock (>lock); > > + > > + /* The shmem_swp_entry() call may have blocked, and > > +* shmem_writepage may have been moving a page between the page > > +* cache and swap cache. We need to recheck the page cache > > +* under the protection of the info->lock spinlock. */ > > + > > + page = find_lock_page(mapping, idx); > > > > Ehh.. Sleeping with the spin-lock held? Sounds like a truly bad idea. > > Umm find_lock_page doesnt sleep does it ? Sure it does. Note the "lock" in find_lock_page(). It will lock the page, which implies sleeping if somebody is accessing it at the same time. If you don't want to sleep, you need to use one of the wrappers for "__find_page_nolock()". Something like "find_get_page()", which only "gets" the page. The naming actually does make sense in this area. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
> On Fri, 23 Mar 2001, Stephen C. Tweedie wrote: > > > > The patch below is for two races in sysV shared memory. > > + spin_lock (>lock); > + > + /* The shmem_swp_entry() call may have blocked, and > +* shmem_writepage may have been moving a page between the page > +* cache and swap cache. We need to recheck the page cache > +* under the protection of the info->lock spinlock. */ > + > + page = find_lock_page(mapping, idx); > > Ehh.. Sleeping with the spin-lock held? Sounds like a truly bad idea. Umm find_lock_page doesnt sleep does it ? Alan - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
On Fri, 23 Mar 2001, Stephen C. Tweedie wrote: > > The patch below is for two races in sysV shared memory. + spin_lock (>lock); + + /* The shmem_swp_entry() call may have blocked, and +* shmem_writepage may have been moving a page between the page +* cache and swap cache. We need to recheck the page cache +* under the protection of the info->lock spinlock. */ + + page = find_lock_page(mapping, idx); Ehh.. Sleeping with the spin-lock held? Sounds like a truly bad idea. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
On Fri, 23 Mar 2001, Stephen C. Tweedie wrote: The patch below is for two races in sysV shared memory. + spin_lock (info-lock); + + /* The shmem_swp_entry() call may have blocked, and +* shmem_writepage may have been moving a page between the page +* cache and swap cache. We need to recheck the page cache +* under the protection of the info-lock spinlock. */ + + page = find_lock_page(mapping, idx); Ehh.. Sleeping with the spin-lock held? Sounds like a truly bad idea. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
On Fri, 23 Mar 2001, Stephen C. Tweedie wrote: The patch below is for two races in sysV shared memory. + spin_lock (info-lock); + + /* The shmem_swp_entry() call may have blocked, and +* shmem_writepage may have been moving a page between the page +* cache and swap cache. We need to recheck the page cache +* under the protection of the info-lock spinlock. */ + + page = find_lock_page(mapping, idx); Ehh.. Sleeping with the spin-lock held? Sounds like a truly bad idea. Umm find_lock_page doesnt sleep does it ? Alan - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
On Fri, 23 Mar 2001, Alan Cox wrote: On Fri, 23 Mar 2001, Stephen C. Tweedie wrote: The patch below is for two races in sysV shared memory. + spin_lock (info-lock); + + /* The shmem_swp_entry() call may have blocked, and +* shmem_writepage may have been moving a page between the page +* cache and swap cache. We need to recheck the page cache +* under the protection of the info-lock spinlock. */ + + page = find_lock_page(mapping, idx); Ehh.. Sleeping with the spin-lock held? Sounds like a truly bad idea. Umm find_lock_page doesnt sleep does it ? It certainly does. find_lock_page() - __find_lock_page() - lock_page() - - __lock_page() - schedule(). - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
On Fri, 23 Mar 2001, Alan Cox wrote: + spin_lock (info-lock); + + /* The shmem_swp_entry() call may have blocked, and +* shmem_writepage may have been moving a page between the page +* cache and swap cache. We need to recheck the page cache +* under the protection of the info-lock spinlock. */ + + page = find_lock_page(mapping, idx); Ehh.. Sleeping with the spin-lock held? Sounds like a truly bad idea. Umm find_lock_page doesnt sleep does it ? Sure it does. Note the "lock" in find_lock_page(). It will lock the page, which implies sleeping if somebody is accessing it at the same time. If you don't want to sleep, you need to use one of the wrappers for "__find_page_nolock()". Something like "find_get_page()", which only "gets" the page. The naming actually does make sense in this area. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
+ page = find_lock_page(mapping, idx); Ehh.. Sleeping with the spin-lock held? Sounds like a truly bad idea. Umm find_lock_page doesnt sleep does it ? It certainly does. find_lock_page() - __find_lock_page() - lock_page() - - __lock_page() - schedule(). Ok I missed the lock page one. Yep. Alan - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
If you don't want to sleep, you need to use one of the wrappers for "__find_page_nolock()". Something like "find_get_page()", which only "gets" the page. * a rather lightweight function, finding and getting a reference to a * hashed page atomically, waiting for it if it's locked. __find_get_page has I think a misleading comment ? The naming actually does make sense in this area. Yep - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
On Fri, 23 Mar 2001, Alan Cox wrote: __find_get_page has I think a misleading comment ? Ehh.. I only said the _naming_ makes sense. [ Wild hand-waving ] I suspect that what happened was that we split off the functions (one to just get the page, one to lock it), and the comment that was associated with the original "find_page()" never got removed, and just happens to sit above one of the helper functions now - the one that didn't lock. I'll fix the comment. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix races in 2.4.2-ac22 SysV shared memory
Alan Cox writes: Umm find_lock_page doesnt sleep does it ? It does lock_page, which sleeps to get the lock if necessary. Later, David S. Miller [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/