On 07/15/2014 09:26 PM, Hugh Dickins wrote:
>>
>> > @@ -760,7 +760,7 @@ static int shmem_writepage(struct page *
>> > spin_lock(&inode->i_lock);
>> > shmem_falloc = inode->i_private;
>>
>> Without ACCESS_ONCE, can shmem_falloc potentially become an alias on
>> inode->i_private and later become re-read outside of the lock?
>
> No, it could be re-read inside the locked section (which is okay since
> the locking ensures the same value would be re-read each time), but it
> cannot be re-read after the unlock. The unlock guarantees that (whereas
> an assignment after the unlock might be moved up before the unlock).
>
> I searched for a simple example (preferably not in code written by me!)
> to convince you. I thought it would be easy to find an example of
>
> spin_lock(&lock);
> thing_to_free = whatever;
> spin_unlock(&lock);
> if (thing_to_free)
> free(thing_to_free);
>
> but everything I hit upon was actually a little more complicated than
> than that (e.g. involving whatever(), or setting whatever = NULL after),
> and therefore less convincing. Please hunt around to convince yourself.
Yeah, I thought myself on the way home that this is probably the case. I guess
some recent bugs made me too paranoid. Sorry for the noise and time you spent
explaining this :/
>>
>> > - if (!shmem_falloc ||
>> > - shmem_falloc->mode != FALLOC_FL_PUNCH_HOLE ||
>> > - vmf->pgoff < shmem_falloc->start ||
>> > - vmf->pgoff >= shmem_falloc->next)
>> > - shmem_falloc = NULL;
>> > - spin_unlock(&inode->i_lock);
>> > - /*
>> > - * i_lock has protected us from taking shmem_falloc seriously
>> > - * once return from shmem_fallocate() went back up that
>> > stack.
>> > - * i_lock does not serialize with i_mutex at all, but it does
>> > - * not matter if sometimes we wait unnecessarily, or
>> > sometimes
>> > - * miss out on waiting: we just need to make those cases
>> > rare.
>> > - */
>> > - if (shmem_falloc) {
>> > + if (shmem_falloc &&
>> > + shmem_falloc->waitq &&
>>
>> Here it's operating outside of lock.
>
> No, it's inside the lock: just easier to see from the patched source
> than from the patch itself.
Ah, right :/
> Hugh
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
