From patchwork Thu May 19 04:48:25 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Manish Katiyar X-Patchwork-Id: 96311 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 20D9A1007D5 for ; Thu, 19 May 2011 14:48:48 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752447Ab1ESEsq (ORCPT ); Thu, 19 May 2011 00:48:46 -0400 Received: from mail-qw0-f46.google.com ([209.85.216.46]:44934 "EHLO mail-qw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751782Ab1ESEsq convert rfc822-to-8bit (ORCPT ); Thu, 19 May 2011 00:48:46 -0400 Received: by qwk3 with SMTP id 3so1166243qwk.19 for ; Wed, 18 May 2011 21:48:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type:content-transfer-encoding; bh=BsbtxQPwWJJBde0Z87Zcg9lDZHOej/MWiEA+VAlC3Jw=; b=cSlho+KJ+oGFP4RK/hu93o2K6mXrIev5q+jKfNI8q2ouOG6nWGFBc3jDqq23rhWlZe wkdt0pwe/TMr+3gO8o8schuVzja2Esl13AWVzIJ6m041C+kaKqZwIL4O0zueowBvnOyX quIF7Jahx9b9LNUlN34ElJqmoIkKyhfnidvbc= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type:content-transfer-encoding; b=MpFsbFI2Plwhui+ziaM0p1ctjGX5HfaFFcR3VPpenBTOQN+dOjujxwkLoR38SCoiE8 atM/oHt0Fa7PIBGHnEHqie6fmjgFmFOjA5FX1FWrFqy2u6CvSXf0SORQSjKY/gTzDSIT qVK1AZOS7Nzvnr819Xy5nGOLg+AGUfci1mx+o= Received: by 10.229.75.196 with SMTP id z4mr2044386qcj.277.1305780525210; Wed, 18 May 2011 21:48:45 -0700 (PDT) MIME-Version: 1.0 Received: by 10.229.230.85 with HTTP; Wed, 18 May 2011 21:48:25 -0700 (PDT) In-Reply-To: <20110518124447.GH25632@quack.suse.cz> References: <20110511155447.GH5057@quack.suse.cz> <20110516155138.GI5344@quack.suse.cz> <20110518080930.GC25632@quack.suse.cz> <20110518124447.GH25632@quack.suse.cz> From: Manish Katiyar Date: Wed, 18 May 2011 21:48:25 -0700 Message-ID: Subject: Re: [PATCH 1/5] jbd2: Pass extra bool parameter in journal routines to specify if its ok to fail the journal transaction allocation. To: Jan Kara Cc: ext4 , "Theodore Ts'o" Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org On Wed, May 18, 2011 at 5:44 AM, Jan Kara wrote: > On Wed 18-05-11 01:36:48, Manish Katiyar wrote: >> On Wed, May 18, 2011 at 1:09 AM, Jan Kara wrote: >> > On Tue 17-05-11 23:38:05, Manish Katiyar wrote: >> >> On Mon, May 16, 2011 at 8:51 AM, Jan Kara wrote: >> >> >  Hello, >> >> > >> >> > On Thu 12-05-11 23:37:05, Manish Katiyar wrote: >> >> >> On Wed, May 11, 2011 at 8:54 AM, Jan Kara wrote: >> >> >> >  Hi, >> >> >> > >> >> >> >  sorry I got to your patches with a delay. One general note - please do >> >> >> > not attach patches. It is enough to have them in the email... >> >> >> > >> >> >> > On Sun 24-04-11 17:10:41, Manish Katiyar wrote: >> >> >> >> Pass extra bool parameter in journal routines to specify if its ok to >> >> >> >> fail the journal transaction allocation. If 'true' is passed >> >> >> >> transaction allocation is done through GFP_KERNEL  and ENOMEM is >> >> >> >> returned else GFP_NOFS is used. >> >> >> >  Please, do not mix error handling with gfp masks. Instead just rename >> >> >> > jbd2__journal_start() to jbd2_journal_start() and change gfp_mask parameter >> >> >> > to "bool errok". >> >> >> >> >> >> ok. >> >> >> >> >> >> > Use GFP_NOFS gfp mask for start_this_handle(). >> >> >> I think I didn't completely understand this line. You meant passing >> >> >> GFP_KERNEL or GFP_NOFS based on errok right ? >> >> >  No, I meant passing GFP_NOFS always. Currently, GFP_NOFS is used in all >> >> > the cases (noone uses GFP_KERNEL variant) and GFP_KERNEL can really be used >> >> > only when we do not hold other filesystem locks (as GFP_KERNEL allocation >> >> > can recurse back into filesystem to reclaim memory). So using GFP_KERNEL >> >> > would need more auditting and is a separate issue anyway. >> >> >> >> Hi Jan, >> >> >> >> How about this ? As suggested I have removed special handlers for >> >> ocfs2, always pass false as default for allocation and have removed >> >> jbd2__journal_start and collapsed it in jbd2_journal_start. >> >> >> >> >> >> Pass extra bool parameter in journal routines to specify if its ok to >> >> fail the journal transaction allocation.  Update ocfs2 and ext4 routines to >> >> pass false for the updated journal interface. >> >  Yes, now the patch looks good! Thanks. Just one nit below: >> >> Thanks a lot. Fixed the newline issue. Here is the updated diff. >> >> Pass extra bool parameter in journal routines to specify if its ok to >> fail the journal transaction allocation.  Update ocfs2 and ext4 routines to >> pass false for the updated journal interface. >  The patch look good now. You can add: > Acked-by: Jan Kara > >                                                                        Honza Signed-off-by: Manish Katiyar Acked-by: Jan Kara --- fs/ext4/ext4_jbd2.h | 2 +- fs/ext4/super.c | 2 +- fs/jbd2/transaction.c | 24 +++++------------------- fs/ocfs2/journal.c | 6 +++--- include/linux/jbd2.h | 6 ++---- 5 files changed, 12 insertions(+), 28 deletions(-) diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h index d0f5353..0abee1b 100644 --- a/fs/ext4/ext4_jbd2.h +++ b/fs/ext4/ext4_jbd2.h @@ -225,7 +225,7 @@ static inline int ext4_journal_extend(handle_t *handle, int nblocks) static inline int ext4_journal_restart(handle_t *handle, int nblocks) { if (ext4_handle_valid(handle)) - return jbd2_journal_restart(handle, nblocks); + return jbd2_journal_restart(handle, nblocks, false); return 0; } diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 8553dfb..4e4c17f 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -279,7 +279,7 @@ handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks) ext4_abort(sb, "Detected aborted journal"); return ERR_PTR(-EROFS); } - return jbd2_journal_start(journal, nblocks); + return jbd2_journal_start(journal, nblocks, false); } /* diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c index 05fa77a..b5c2550 100644 --- a/fs/jbd2/transaction.c +++ b/fs/jbd2/transaction.c @@ -318,7 +318,7 @@ static handle_t *new_handle(int nblocks) * * Return a pointer to a newly allocated handle, or NULL on failure */ -handle_t *jbd2__journal_start(journal_t *journal, int nblocks, int gfp_mask) +handle_t *jbd2_journal_start(journal_t *journal, int nblocks, bool errok) { handle_t *handle = journal_current_handle(); int err; @@ -338,7 +338,7 @@ handle_t *jbd2__journal_start(journal_t *journal, int nblocks, int gfp_mask) current->journal_info = handle; - err = start_this_handle(journal, handle, gfp_mask); + err = start_this_handle(journal, handle, GFP_NOFS); if (err < 0) { jbd2_free_handle(handle); current->journal_info = NULL; @@ -346,13 +346,6 @@ handle_t *jbd2__journal_start(journal_t *journal, int nblocks, int gfp_mask) } return handle; } -EXPORT_SYMBOL(jbd2__journal_start); - - -handle_t *jbd2_journal_start(journal_t *journal, int nblocks) -{ - return jbd2__journal_start(journal, nblocks, GFP_NOFS); -} EXPORT_SYMBOL(jbd2_journal_start); @@ -441,7 +434,7 @@ out: * transaction capabable of guaranteeing the requested number of * credits. */ -int jbd2__journal_restart(handle_t *handle, int nblocks, int gfp_mask) +int jbd2_journal_restart(handle_t *handle, int nblocks, bool errok) { transaction_t *transaction = handle->h_transaction; journal_t *journal = transaction->t_journal; @@ -477,16 +470,9 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, int gfp_mask) lock_map_release(&handle->h_lockdep_map); handle->h_buffer_credits = nblocks; - ret = start_this_handle(journal, handle, gfp_mask); + ret = start_this_handle(journal, handle, GFP_NOFS); return ret; } -EXPORT_SYMBOL(jbd2__journal_restart); - - -int jbd2_journal_restart(handle_t *handle, int nblocks) -{ - return jbd2__journal_restart(handle, nblocks, GFP_NOFS); -} EXPORT_SYMBOL(jbd2_journal_restart); /** @@ -1436,7 +1422,7 @@ int jbd2_journal_force_commit(journal_t *journal) handle_t *handle; int ret; - handle = jbd2_journal_start(journal, 1); + handle = jbd2_journal_start(journal, 1, false); if (IS_ERR(handle)) { ret = PTR_ERR(handle); } else { diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c index b141a44..cca0f76 100644 --- a/fs/ocfs2/journal.c +++ b/fs/ocfs2/journal.c @@ -353,11 +353,11 @@ handle_t *ocfs2_start_trans(struct ocfs2_super *osb, int max_buffs) /* Nested transaction? Just return the handle... */ if (journal_current_handle()) - return jbd2_journal_start(journal, max_buffs); + return jbd2_journal_start(journal, max_buffs, false); down_read(&osb->journal->j_trans_barrier); - handle = jbd2_journal_start(journal, max_buffs); + handle = jbd2_journal_start(journal, max_buffs, false); if (IS_ERR(handle)) { up_read(&osb->journal->j_trans_barrier); @@ -438,7 +438,7 @@ int ocfs2_extend_trans(handle_t *handle, int nblocks) if (status > 0) { trace_ocfs2_extend_trans_restart(old_nblocks + nblocks); status = jbd2_journal_restart(handle, - old_nblocks + nblocks); + old_nblocks + nblocks, false); if (status < 0) { mlog_errno(status); goto bail; diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index a32dcae..67f0f4f 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -1103,10 +1103,8 @@ static inline handle_t *journal_current_handle(void) * Register buffer modifications against the current transaction. */ -extern handle_t *jbd2_journal_start(journal_t *, int nblocks); -extern handle_t *jbd2__journal_start(journal_t *, int nblocks, int gfp_mask); -extern int jbd2_journal_restart(handle_t *, int nblocks); -extern int jbd2__journal_restart(handle_t *, int nblocks, int gfp_mask); +extern handle_t *jbd2_journal_start(journal_t *, int nblocks, bool errok); +extern int jbd2_journal_restart(handle_t *, int nblocks, bool errok); extern int jbd2_journal_extend (handle_t *, int nblocks); extern int jbd2_journal_get_write_access(handle_t *, struct buffer_head *); extern int jbd2_journal_get_create_access (handle_t *, struct buffer_head *);