Hi Jan,
This is the follow up from https://lkml.org/lkml/2011/1/17/154
Following patches make jbd2 to use GFP_KERNEL for transaction
allocation if the caller can handle the errors. Following is the list
of functions that I updated to pass the new flag. Also below is the
list of functions which still have the old behavior and pass the old
flags (either because they can't deal with errors, or I wasn't too
sure so I did conservatively). Appreciate your feedback. The other
callers of jbd2_journal_start() are from ocfs2, they still pass the
old flag.
OK to handle errors.
-------------------------
ext4_init_inode_table
ext4_group_add
setup_new_group_blocks
update_backups
ext4_group_extend
ext4_release_dquot
ext4_acquire_dquot
ext4_acl_chmod
ext4_xattr_set_acl
ext4_xattr_set
et4_ioctl - For setting EXT4_IOC_SETVERSION_OLD
ext4_ext_migrate
ext4_ext_migrate
ext4_rmdir
ext4_unlink
ext4_symlink
ext4_link
ext4_rename
ext4_mkdir
ext4_mknod
ext4_fallocate
ext4_create
move_extent_per_page
ext4_change_inode_journal_flag
ext4_setattr
ext4_setattr
NOT ok
-------------------
ext4_write_dquot
ext4_ioctl - Not ok for EXT4_EOFBLOCKS_FL
ext4_ext_remove_space
ext4_ext_truncate
ext4_write_info
ext4_convert_unwritten_extents
ext4_ind_direct_IO
ext4_ind_direct_IO
ext4_dirty_inode
ext4_setattr
ext4_evict_inode
_ext4_get_block
ext4_write_begin
__ext4_journalled_writepage
ext4_da_writepages
ext4_da_write_begin
start_transaction
Signed-off-by: Manish Katiyar <[email protected]>
---
fs/jbd2/transaction.c | 12 +++++++-----
include/linux/jbd2.h | 4 ++--
2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index faad2bd..0fa4c86 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -346,9 +346,10 @@ handle_t *jbd2__journal_start(journal_t *journal,
int nblocks, int gfp_mask)
EXPORT_SYMBOL(jbd2__journal_start);
-handle_t *jbd2_journal_start(journal_t *journal, int nblocks)
+handle_t *jbd2_journal_start(journal_t *journal, int nblocks, bool errok)
{
- return jbd2__journal_start(journal, nblocks, GFP_NOFS);
+ return jbd2__journal_start(journal, nblocks,
+ errok ? GFP_KERNEL : GFP_NOFS);
}
EXPORT_SYMBOL(jbd2_journal_start);
@@ -476,9 +477,10 @@ int jbd2__journal_restart(handle_t *handle, int
nblocks, int gfp_mask)
EXPORT_SYMBOL(jbd2__journal_restart);
-int jbd2_journal_restart(handle_t *handle, int nblocks)
+int jbd2_journal_restart(handle_t *handle, int nblocks, bool errok)
{
- return jbd2__journal_restart(handle, nblocks, GFP_NOFS);
+ return jbd2__journal_restart(handle, nblocks,
+ errok ? GFP_KERNEL : GFP_NOFS);
}
EXPORT_SYMBOL(jbd2_journal_restart);
@@ -1429,7 +1431,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/include/linux/jbd2.h b/include/linux/jbd2.h
index 27e79c2..b24342f 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1081,9 +1081,9 @@ 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, bool errok);
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, bool errok);
extern int jbd2__journal_restart(handle_t *, int nblocks, int gfp_mask);
extern int jbd2_journal_extend (handle_t *, int nblocks);
extern int jbd2_journal_get_write_access(handle_t *, struct buffer_head *);
--
1.6.0.4
--
Thanks -
Manish
==================================
[$\*.^ -- I miss being one of them
==================================
On Sat, Jan 22, 2011 at 07:32:44PM -0800, Manish Katiyar wrote:
> Hi Jan,
>
> This is the follow up from https://lkml.org/lkml/2011/1/17/154
> Following patches make jbd2 to use GFP_KERNEL for transaction
> allocation if the caller can handle the errors. Following is the list
> of functions that I updated to pass the new flag. Also below is the
> list of functions which still have the old behavior and pass the old
> flags (either because they can't deal with errors, or I wasn't too
> sure so I did conservatively). Appreciate your feedback. The other
> callers of jbd2_journal_start() are from ocfs2, they still pass the
> old flag.
Hmm, I wonder if it would be better to use
jbd2_journal_start(...)
and
jbd2_journal_start_nofail(...)
The tradeoff is that long-term, the code is more readable (as opposed
to having people look up what a random "true" or "false" value means).
But short-term, while it will make the patch smaller, it also makes
the patch harder audit, since we need to look at all of the places
where we _haven't_ made a change to make sure those call sites can
tolerate an error return.
- Ted
On Sat, Jan 22, 2011 at 9:40 PM, Ted Ts'o <[email protected]> wrote:
> On Sat, Jan 22, 2011 at 07:32:44PM -0800, Manish Katiyar wrote:
>> ?Hi Jan,
>>
>> This is the follow up from https://lkml.org/lkml/2011/1/17/154
>> Following patches make jbd2 to use GFP_KERNEL for transaction
>> allocation if the caller can handle the errors. Following is the list
>> of functions that I updated to pass the new flag. Also below is the
>> list of functions which still have the old behavior and pass the old
>> flags (either because they can't deal with errors, or I wasn't too
>> sure so I did conservatively). Appreciate your feedback. The other
>> callers of jbd2_journal_start() are from ocfs2, they still pass the
>> old flag.
>
> Hmm, I wonder if it would be better to use
>
> jbd2_journal_start(...)
>
> and
>
> jbd2_journal_start_nofail(...)
>
> The tradeoff is that long-term, the code is more readable (as opposed
> to having people look up what a random "true" or "false" value means).
> But short-term, while it will make the patch smaller, it also makes
> the patch harder audit, since we need to look at all of the places
> where we _haven't_ made a change to make sure those call sites can
> tolerate an error return.
Yes, a new interface is better but wasn't too sure. If the reviewers
feel that is the way to go I can redo. Infact if
this version makes review easier, then once these changes look ok and
agreed, I can submit an updated version with these
changes applied using the new interface. Will that make sense ?
--
Thanks -
Manish
On Sun, Jan 23, 2011 at 12:40:49AM -0500, Ted Ts'o wrote:
> On Sat, Jan 22, 2011 at 07:32:44PM -0800, Manish Katiyar wrote:
> > Hi Jan,
> >
> > This is the follow up from https://lkml.org/lkml/2011/1/17/154
> > Following patches make jbd2 to use GFP_KERNEL for transaction
> > allocation if the caller can handle the errors. Following is the list
> > of functions that I updated to pass the new flag. Also below is the
> > list of functions which still have the old behavior and pass the old
> > flags (either because they can't deal with errors, or I wasn't too
> > sure so I did conservatively). Appreciate your feedback. The other
> > callers of jbd2_journal_start() are from ocfs2, they still pass the
> > old flag.
>
> Hmm, I wonder if it would be better to use
>
> jbd2_journal_start(...)
>
> and
>
> jbd2_journal_start_nofail(...)
This API is markedly better to read. Btw, does _nofail() mean no
possible failures, or just no memory errors? If it is no failures, I'd
love to see the function become void.
> The tradeoff is that long-term, the code is more readable (as opposed
> to having people look up what a random "true" or "false" value means).
> But short-term, while it will make the patch smaller, it also makes
> the patch harder audit, since we need to look at all of the places
> where we _haven't_ made a change to make sure those call sites can
> tolerate an error return.
I think we should start with jbd2_journal_start_can_fail() or
something like it, and change it back to jbd2_journal_start() in the
next window. It's a silly name, but it catches exactly what you are
worried about.
Joel
--
Life's Little Instruction Book #94
"Make it a habit to do nice things for people who
will never find out."
http://www.jlbec.org/
[email protected]
On Sat 22-01-11 22:29:01, Joel Becker wrote:
> On Sun, Jan 23, 2011 at 12:40:49AM -0500, Ted Ts'o wrote:
> > On Sat, Jan 22, 2011 at 07:32:44PM -0800, Manish Katiyar wrote:
> > > Hi Jan,
> > >
> > > This is the follow up from https://lkml.org/lkml/2011/1/17/154
> > > Following patches make jbd2 to use GFP_KERNEL for transaction
> > > allocation if the caller can handle the errors. Following is the list
> > > of functions that I updated to pass the new flag. Also below is the
> > > list of functions which still have the old behavior and pass the old
> > > flags (either because they can't deal with errors, or I wasn't too
> > > sure so I did conservatively). Appreciate your feedback. The other
> > > callers of jbd2_journal_start() are from ocfs2, they still pass the
> > > old flag.
> >
> > Hmm, I wonder if it would be better to use
> >
> > jbd2_journal_start(...)
> >
> > and
> >
> > jbd2_journal_start_nofail(...)
>
> This API is markedly better to read. Btw, does _nofail() mean no
> possible failures, or just no memory errors? If it is no failures, I'd
> love to see the function become void.
jbd2_journal_start can always fail e.g. because the journal is aborted.
So it really just means no memory failures...
> > The tradeoff is that long-term, the code is more readable (as opposed
> > to having people look up what a random "true" or "false" value means).
> > But short-term, while it will make the patch smaller, it also makes
> > the patch harder audit, since we need to look at all of the places
> > where we _haven't_ made a change to make sure those call sites can
> > tolerate an error return.
>
> I think we should start with jbd2_journal_start_can_fail() or
> something like it, and change it back to jbd2_journal_start() in the
> next window. It's a silly name, but it catches exactly what you are
> worried about.
Yes, I think this would be nice for auditting (but for that matter
current interface with additional argument isn't bad either and we can
just do the rename to _nofail in the final patch...).
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon, Jan 24, 2011 at 02:31:43PM +0100, Jan Kara wrote:
> On Sat 22-01-11 22:29:01, Joel Becker wrote:
> > This API is markedly better to read. Btw, does _nofail() mean no
> > possible failures, or just no memory errors? If it is no failures, I'd
> > love to see the function become void.
> jbd2_journal_start can always fail e.g. because the journal is aborted.
> So it really just means no memory failures...
Then _nofail() is a terrible name, because it can still fail.
Let's call it jbd2_journal_start_nofs(); that's what it is.
Joel
--
"Under capitalism, man exploits man. Under Communism, it's just
the opposite."
- John Kenneth Galbraith
http://www.jlbec.org/
[email protected]
On 2011-01-24, at 06:31, Jan Kara wrote:
> On Sat 22-01-11 22:29:01, Joel Becker wrote:
>> On Sun, Jan 23, 2011 at 12:40:49AM -0500, Ted Ts'o wrote:
>>> Hmm, I wonder if it would be better to use
>>>
>>> jbd2_journal_start(...)
>>>
>>> and
>>>
>>> jbd2_journal_start_nofail(...)
>>
>> This API is markedly better to read. Btw, does _nofail() mean no
>> possible failures, or just no memory errors? If it is no failures, I'd
>> love to see the function become void.
I also prefer changing the function name instead of adding an argument.
> jbd2_journal_start can always fail e.g. because the journal is aborted.
> So it really just means no memory failures...
>
>>> The tradeoff is that long-term, the code is more readable (as opposed
>>> to having people look up what a random "true" or "false" value means).
>>> But short-term, while it will make the patch smaller, it also makes
>>> the patch harder audit, since we need to look at all of the places
>>> where we _haven't_ made a change to make sure those call sites can
>>> tolerate an error return.
>>
>> I think we should start with jbd2_journal_start_can_fail() or
>> something like it, and change it back to jbd2_journal_start() in the
>> next window. It's a silly name, but it catches exactly what you are
>> worried about.
>
> Yes, I think this would be nice for auditting (but for that matter
> current interface with additional argument isn't bad either and we can
> just do the rename to _nofail in the final patch...).
The reason I don't like the "true" and "false" arguments is that it isn't at all clear which functions have "false" because they cannot fail, and which ones just haven't been updated yet.
In that light, I'd prefer to add _two_ new functions, one that indicates the function needs to retry (as it does now), and one that indicates that the caller will handle the error. That way it is clear which functions have been investigated, and which ones haven't been looked at yet. Once all of the functions have been changed, we can remove the old jbd2_journal_start() function to catch any patches that have not been updated to the new functions.
Maybe jbd2_journal_start_canfail() and jbd2_journal_start_retry()?
Cheers, Andreas
On 2011-01-24, at 06:31, Jan Kara wrote:
> On Sat 22-01-11 22:29:01, Joel Becker wrote:
>> On Sun, Jan 23, 2011 at 12:40:49AM -0500, Ted Ts'o wrote:
>>> Hmm, I wonder if it would be better to use
>>>
>>> jbd2_journal_start(...)
>>>
>>> and
>>>
>>> jbd2_journal_start_nofail(...)
>>
>> This API is markedly better to read. Btw, does _nofail() mean no
>> possible failures, or just no memory errors? If it is no failures, I'd
>> love to see the function become void.
I also prefer changing the function name instead of adding an argument.
> jbd2_journal_start can always fail e.g. because the journal is aborted.
> So it really just means no memory failures...
>
>>> The tradeoff is that long-term, the code is more readable (as opposed
>>> to having people look up what a random "true" or "false" value means).
>>> But short-term, while it will make the patch smaller, it also makes
>>> the patch harder audit, since we need to look at all of the places
>>> where we _haven't_ made a change to make sure those call sites can
>>> tolerate an error return.
>>
>> I think we should start with jbd2_journal_start_can_fail() or
>> something like it, and change it back to jbd2_journal_start() in the
>> next window. It's a silly name, but it catches exactly what you are
>> worried about.
>
> Yes, I think this would be nice for auditting (but for that matter
> current interface with additional argument isn't bad either and we can
> just do the rename to _nofail in the final patch...).
The reason I don't like the "true" and "false" arguments is that it isn't at all clear which functions have "false" because they cannot fail, and which ones just haven't been updated yet.
In that light, I'd prefer to add _two_ new functions, one that indicates the function needs to retry (as it does now), and one that indicates that the caller will handle the error. That way it is clear which functions have been investigated, and which ones haven't been looked at yet. Once all of the functions have been changed, we can remove the old jbd2_journal_start() function to catch any patches that have not been updated to the new functions.
Maybe jbd2_journal_start_canfail() and jbd2_journal_start_retry()?
Cheers, Andreas
On Mon 24-01-11 09:20:23, Joel Becker wrote:
> On Mon, Jan 24, 2011 at 02:31:43PM +0100, Jan Kara wrote:
> > On Sat 22-01-11 22:29:01, Joel Becker wrote:
> > > This API is markedly better to read. Btw, does _nofail() mean no
> > > possible failures, or just no memory errors? If it is no failures, I'd
> > > love to see the function become void.
> > jbd2_journal_start can always fail e.g. because the journal is aborted.
> > So it really just means no memory failures...
>
> Then _nofail() is a terrible name, because it can still fail.
> Let's call it jbd2_journal_start_nofs(); that's what it is.
Good point. But it's not even NO_FS - that's just an internal thing how
JBD2 abuses gfp flags to communicate it's wish. It should really be "do not
fail if there is still hope because user will silently loose data if you
do". But that's obviously too long for a function name suffix ;). I'm
looking for a better name... Andreas' _retry does not well describe what's
the desired effect either.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon 24-01-11 18:06:24, Andreas Dilger wrote:
> > jbd2_journal_start can always fail e.g. because the journal is aborted.
> > So it really just means no memory failures...
> >
> >>> The tradeoff is that long-term, the code is more readable (as opposed
> >>> to having people look up what a random "true" or "false" value means).
> >>> But short-term, while it will make the patch smaller, it also makes
> >>> the patch harder audit, since we need to look at all of the places
> >>> where we _haven't_ made a change to make sure those call sites can
> >>> tolerate an error return.
> >>
> >> I think we should start with jbd2_journal_start_can_fail() or
> >> something like it, and change it back to jbd2_journal_start() in the
> >> next window. It's a silly name, but it catches exactly what you are
> >> worried about.
> >
> > Yes, I think this would be nice for auditting (but for that matter
> > current interface with additional argument isn't bad either and we can
> > just do the rename to _nofail in the final patch...).
>
> The reason I don't like the "true" and "false" arguments is that it isn't
> at all clear which functions have "false" because they cannot fail, and
> which ones just haven't been updated yet.
>
> In that light, I'd prefer to add _two_ new functions, one that indicates
> the function needs to retry (as it does now), and one that indicates that
> the caller will handle the error. That way it is clear which functions
> have been investigated, and which ones haven't been looked at yet. Once
> all of the functions have been changed, we can remove the old
> jbd2_journal_start() function to catch any patches that have not been
> updated to the new functions.
I agree this would be good for the transition period but once we go
through all the callsites, I'd prefer to do a rename and have just
jbd2_journal_start() be the one which does not retry.
> Maybe jbd2_journal_start_canfail() and jbd2_journal_start_retry()?
As I said above, I'd like the first one to live only temporarily so
I don't care about the name. The second one is probably better than
_nofail() but I still don't feel it describes well what the function
does...
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Tue, Jan 25, 2011 at 3:46 AM, Jan Kara <[email protected]> wrote:
> On Mon 24-01-11 18:06:24, Andreas Dilger wrote:
>> > ?jbd2_journal_start can always fail e.g. because the journal is aborted.
>> > So it really just means no memory failures...
>> >
>> >>> The tradeoff is that long-term, the code is more readable (as opposed
>> >>> to having people look up what a random "true" or "false" value means).
>> >>> But short-term, while it will make the patch smaller, it also makes
>> >>> the patch harder audit, since we need to look at all of the places
>> >>> where we _haven't_ made a change to make sure those call sites can
>> >>> tolerate an error return.
>> >>
>> >> ? ?I think we should start with jbd2_journal_start_can_fail() or
>> >> something like it, and change it back to jbd2_journal_start() in the
>> >> next window. ?It's a silly name, but it catches exactly what you are
>> >> worried about.
>> >
>> > ?Yes, I think this would be nice for auditting (but for that matter
>> > current interface with additional argument isn't bad either and we can
>> > just do the rename to _nofail in the final patch...).
>>
>> The reason I don't like the "true" and "false" arguments is that it isn't
>> at all clear which functions have "false" because they cannot fail, and
>> which ones just haven't been updated yet.
>>
>> In that light, I'd prefer to add _two_ new functions, one that indicates
>> the function needs to retry (as it does now), and one that indicates that
>> the caller will handle the error. ?That way it is clear which functions
>> have been investigated, and which ones haven't been looked at yet. ?Once
>> all of the functions have been changed, we can remove the old
>> jbd2_journal_start() function to catch any patches that have not been
>> updated to the new functions.
> ?I agree this would be good for the transition period but once we go
> through all the callsites, I'd prefer to do a rename and have just
> jbd2_journal_start() be the one which does not retry.
>
>> Maybe jbd2_journal_start_canfail() and jbd2_journal_start_retry()?
> ?As I said above, I'd like the first one to live only temporarily so
> I don't care about the name. The second one is probably better than
> _nofail() but I still don't feel it describes well what the function
> does...
Hi all,
Have we reached on any conclusion yet on the function name which I can
use to send my updated patch ? My preference from the above list is to
use ext4_journal_start_nofs() as that seems the closest match, but I
would like hear the conclusion from experts.
--
Thanks -
Manish
On Sat 29-01-11 21:40:03, Manish Katiyar wrote:
Hi Manish,
> Have we reached on any conclusion yet on the function name which I can
> use to send my updated patch ? My preference from the above list is to
> use ext4_journal_start_nofs() as that seems the closest match, but I
> would like hear the conclusion from experts.
How about "ext4_journal_start_tryhard()"? I don't like "nofs" because
the fact whether we use GFP_NOFS is separate from the fact whether we are
able to handle memory allocation failure.
In fact, your patch set should keep calling kzalloc() with GFP_NOFS in all
cases to avoid new recursions into the filesystem (which could cause
deadlocks - changing that would need a separate audit if we care enough).
Just the retrying logic should be influenced by your work.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Fri, Feb 4, 2011 at 7:53 AM, Jan Kara <[email protected]> wrote:
> On Sat 29-01-11 21:40:03, Manish Katiyar wrote:
> ?Hi Manish,
>
>> Have we reached on any conclusion yet on the function name which I can
>> use to send my updated patch ? My preference from the above list is to
>> use ext4_journal_start_nofs() as that seems the closest match, but I
>> would like hear the conclusion from experts.
> ?How about "ext4_journal_start_tryhard()"?
Thanks Jan,
I will post an updated patch of the full series using
ext4_journal_start_tryhard() once other patches have been reviewed.
--
Thanks -
Manish
On Fri, Feb 4, 2011 at 7:53 AM, Jan Kara <[email protected]> wrote:
> On Sat 29-01-11 21:40:03, Manish Katiyar wrote:
> ?Hi Manish,
>
>> Have we reached on any conclusion yet on the function name which I can
>> use to send my updated patch ? My preference from the above list is to
>> use ext4_journal_start_nofs() as that seems the closest match, but I
>> would like hear the conclusion from experts.
> ?How about "ext4_journal_start_tryhard()"? I don't like "nofs" because
> the fact whether we use GFP_NOFS is separate from the fact whether we are
> able to handle memory allocation failure.
Hi Jan,
Sorry for taking so long to respond to this. I'm sending the updated
set of patches after incorporating your previous comments. As
suggested I have added a wrapper routine ext4_journal_start_tryhard()
for the transaction allocation which can't fail and does
its allocation using GFP_NOFS. Regular ext4_journal_start() uses GFP_KERNEL.
--
Thanks a lot -
Manish