LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Oleg Nesterov <oleg@tv-sign.ru>
To: Hugh Dickins <hugh@veritas.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Miklos Szeredi <mszeredi@suse.cz>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mmap_region: cleanup, remove unneeded file != NULL check
Date: Mon, 11 Feb 2008 13:34:05 +0300	[thread overview]
Message-ID: <20080211103405.GB162@tv-sign.ru> (raw)
In-Reply-To: <Pine.LNX.4.64.0802062013210.32204@blonde.site>

Sorry for delay,

On 02/06, Hugh Dickins wrote:
>
> On Sun, 3 Feb 2008, Oleg Nesterov wrote:
> 
> > mmap_region() checks "file != NULL" when we know "file && vma_merge() == T".
> > Also, swap these if/else branches, imho make the code a bit more readable.
> > 
> > Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
> 
> Acked-with-a-but-by: Hugh Dickins <hugh@veritas.com>
> 
> But my but is this: you can go one step further, it's silly to be
> repeating the "if (correct_wcount) atomic_inc..." in both the if
> and the else clauses.

Ah. Shame on me. Of course, I noticed these 2 correct_wcount's, but
didn't realize we can safely use "inode" after fput(). Not only the
caller should have a reference, vma_merge() requires that vm_file == file,
so fput(file) can't destroy the last reference.

> For several minutes I thought that must indicate we already had a
> bug there.  Eventually I realized not: we need deny_write_access()
> above to test and deny atomically, then once we've merged or linked
> the vma it's securely denying in the vma itself: so before returning
> we need to undo our temporary denial.  A brief comment might be
> worthwhile, perhaps something like
> 
> 	/* Once vma denies write, undo our temporary denial count */
> 	if (correct_wcount)
> 		atomic_inc(&inode->i_writecount);

Thanks Hugh, I'll redo this cleanup.

Oleg.


      reply	other threads:[~2008-02-11 10:34 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-03 18:01 Oleg Nesterov
2008-02-06 20:22 ` Hugh Dickins
2008-02-11 10:34   ` Oleg Nesterov [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080211103405.GB162@tv-sign.ru \
    --to=oleg@tv-sign.ru \
    --cc=akpm@linux-foundation.org \
    --cc=hugh@veritas.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mszeredi@suse.cz \
    --subject='Re: [PATCH] mmap_region: cleanup, remove unneeded file '\!'= NULL check' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).