From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: with ECARTIS (v1.0.0; list xfs); Fri, 15 Dec 2006 10:54:19 -0800 (PST) Received: from mga01.intel.com ([192.55.52.88]) by oss.sgi.com (8.12.10/8.12.10/SuSE Linux 0.7) with ESMTP id kBFIs8qw024312 for ; Fri, 15 Dec 2006 10:54:10 -0800 From: "Chen, Kenneth W" Subject: RE: [PATCH] incorrect error handling inside generic_file_direct_write Date: Fri, 15 Dec 2006 10:53:18 -0800 Message-ID: <000101c7207a$48c138f0$ff0da8c0@amr.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit In-Reply-To: <20061215104341.GA20089@infradead.org> Sender: xfs-bounce@oss.sgi.com Errors-to: xfs-bounce@oss.sgi.com List-Id: xfs To: 'Christoph Hellwig' Cc: 'Andrew Morton' , Dmitriy Monakhov , Dmitriy Monakhov , linux-kernel@vger.kernel.org, Linux Memory Management , devel@openvz.org, xfs@oss.sgi.com Christoph Hellwig wrote on Friday, December 15, 2006 2:44 AM > So we're doing the sync_page_range once in __generic_file_aio_write > with i_mutex held. > > > > mutex_lock(&inode->i_mutex); > > - ret = __generic_file_aio_write_nolock(iocb, iov, nr_segs, > > - &iocb->ki_pos); > > + ret = __generic_file_aio_write(iocb, iov, nr_segs, pos); > > mutex_unlock(&inode->i_mutex); > > > > if (ret > 0 && ((file->f_flags & O_SYNC) || IS_SYNC(inode))) { > > And then another time after it's unlocked, this seems wrong. I didn't invent that mess though. I should've ask the question first: in 2.6.20-rc1, generic_file_aio_write will call sync_page_range twice, once from __generic_file_aio_write_nolock and once within the function itself. Is it redundant? Can we delete the one in the top level function? Like the following? --- ./mm/filemap.c.orig 2006-12-15 09:02:58.000000000 -0800 +++ ./mm/filemap.c 2006-12-15 09:03:19.000000000 -0800 @@ -2370,14 +2370,6 @@ ssize_t generic_file_aio_write(struct ki ret = __generic_file_aio_write_nolock(iocb, iov, nr_segs, &iocb->ki_pos); mutex_unlock(&inode->i_mutex); - - if (ret > 0 && ((file->f_flags & O_SYNC) || IS_SYNC(inode))) { - ssize_t err; - - err = sync_page_range(inode, mapping, pos, ret); - if (err < 0) - ret = err; - } return ret; } EXPORT_SYMBOL(generic_file_aio_write);