Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755863AbYANL4q (ORCPT ); Mon, 14 Jan 2008 06:56:46 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1755112AbYANL4g (ORCPT ); Mon, 14 Jan 2008 06:56:36 -0500 Received: from py-out-1112.google.com ([64.233.166.180]:65083 "EHLO py-out-1112.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754596AbYANL4f (ORCPT ); Mon, 14 Jan 2008 06:56:35 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references; b=uKnp6HRcTJgxBY+9UnK/xoQjlL9W0FfSj+qGsYO6LrQXla6/A6pxoQEtsBuAHOWkb0NeiTy3PB94MW9IqtShf2jevUmOoUnBJbzHDgr5gBo37QTnvB2cdmGIaDhVt4hFPsSRYJZJl3fkWr/02mbXJpHH7LyjoXF/zw323t3iqBI= Message-ID: <4df4ef0c0801140356x2bf11e33oc8969d65ad5641a6@mail.gmail.com> Date: Mon, 14 Jan 2008 14:56:33 +0300 From: "Anton Salikhmetov" To: "Miklos Szeredi" Subject: Re: [PATCH 1/2] massive code cleanup of sys_msync() Cc: linux-mm@kvack.org, jakob@unthought.net, linux-kernel@vger.kernel.org, valdis.kletnieks@vt.edu, riel@redhat.com, ksm@42.dk, staubach@redhat.com, jesper.juhl@gmail.com, torvalds@linux-foundation.org, a.p.zijlstra@chello.nl, akpm@linux-foundation.org, protasnb@gmail.com In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <12001991991217-git-send-email-salikhmetov@gmail.com> <12001992013606-git-send-email-salikhmetov@gmail.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5992 Lines: 170 2008/1/14, Miklos Szeredi : > > Substantial code cleanup of the sys_msync() function: > > > > 1) using the PAGE_ALIGN() macro instead of "manual" alignment; > > 2) improved readability of the loop traversing the process memory regions. > > Thanks for doing this. See comments below. > > > Signed-off-by: Anton Salikhmetov > > --- > > mm/msync.c | 78 +++++++++++++++++++++++++++--------------------------------- > > 1 files changed, 35 insertions(+), 43 deletions(-) > > > > diff --git a/mm/msync.c b/mm/msync.c > > index 144a757..ff654c9 100644 > > --- a/mm/msync.c > > +++ b/mm/msync.c > > @@ -1,24 +1,25 @@ > > /* > > * linux/mm/msync.c > > * > > + * The msync() system call. > > * Copyright (C) 1994-1999 Linus Torvalds > > + * > > + * Substantial code cleanup. > > + * Copyright (C) 2008 Anton Salikhmetov > > */ > > > > -/* > > - * The msync() system call. > > - */ > > +#include > > #include > > #include > > #include > > -#include > > -#include > > #include > > +#include > > > > /* > > * MS_SYNC syncs the entire file - including mappings. > > * > > * MS_ASYNC does not start I/O (it used to, up to 2.5.67). > > - * Nor does it marks the relevant pages dirty (it used to up to 2.6.17). > > + * Nor does it mark the relevant pages dirty (it used to up to 2.6.17). > > * Now it doesn't do anything, since dirty pages are properly tracked. > > * > > * The application may now run fsync() to > > @@ -33,71 +34,62 @@ asmlinkage long sys_msync(unsigned long start, size_t len, int flags) > > unsigned long end; > > struct mm_struct *mm = current->mm; > > struct vm_area_struct *vma; > > - int unmapped_error = 0; > > - int error = -EINVAL; > > + int error = 0, unmapped_error = 0; > > > > if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC)) > > - goto out; > > + return -EINVAL; > > if (start & ~PAGE_MASK) > > - goto out; > > + return -EINVAL; > > if ((flags & MS_ASYNC) && (flags & MS_SYNC)) > > - goto out; > > - error = -ENOMEM; > > - len = (len + ~PAGE_MASK) & PAGE_MASK; > > + return -EINVAL; > > + > > + len = PAGE_ALIGN(len); > > end = start + len; > > if (end < start) > > - goto out; > > - error = 0; > > + return -ENOMEM; > > if (end == start) > > - goto out; > > + return 0; > > + > > /* > > * If the interval [start,end) covers some unmapped address ranges, > > * just ignore them, but return -ENOMEM at the end. > > */ > > down_read(&mm->mmap_sem); > > vma = find_vma(mm, start); > > - for (;;) { > > + do { > > struct file *file; > > > > - /* Still start < end. */ > > - error = -ENOMEM; > > - if (!vma) > > - goto out_unlock; > > - /* Here start < vma->vm_end. */ > > + if (!vma) { > > + error = -ENOMEM; > > + break; > > + } > > if (start < vma->vm_start) { > > start = vma->vm_start; > > - if (start >= end) > > - goto out_unlock; > > + if (start >= end) { > > + error = -ENOMEM; > > + break; > > + } > > unmapped_error = -ENOMEM; > > } > > - /* Here vma->vm_start <= start < vma->vm_end. */ > > - if ((flags & MS_INVALIDATE) && > > - (vma->vm_flags & VM_LOCKED)) { > > + if ((flags & MS_INVALIDATE) && (vma->vm_flags & VM_LOCKED)) { > > error = -EBUSY; > > - goto out_unlock; > > + break; > > } > > file = vma->vm_file; > > - start = vma->vm_end; > > - if ((flags & MS_SYNC) && file && > > - (vma->vm_flags & VM_SHARED)) { > > + if ((flags & MS_SYNC) && file && (vma->vm_flags & VM_SHARED)) { > > get_file(file); > > up_read(&mm->mmap_sem); > > error = do_fsync(file, 0); > > fput(file); > > - if (error || start >= end) > > - goto out; > > + if (error) > > + return error; > > down_read(&mm->mmap_sem); > > - vma = find_vma(mm, start); > > Where did this line go? It's needed because after releasing and > reacquiring the mmap sem, the old vma may have gone away. > > I suggest, that when doing such a massive cleanup, that you split it > up even further into easily understandable pieces, so such bugs cannot > creep in. Thanks for your review. I overlooked this problem. I'll redo my cleanup soon. > > > - } else { > > - if (start >= end) { > > - error = 0; > > - goto out_unlock; > > - } > > - vma = vma->vm_next; > > } > > - } > > -out_unlock: > > + > > + start = vma->vm_end; > > + vma = vma->vm_next; > > + } while (start < end); > > up_read(&mm->mmap_sem); > > -out: > > + > > return error ? : unmapped_error; > > } > > -- > > 1.4.4.4 > > Miklos > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/