| Message ID | AANLkTi=KXX9zjBKSGs18OpZUzOD-yyKj9hNVSBi_iBAB@mail.gmail.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Tue, Mar 01, 2011 at 03:30:24PM +0200, Amir Goldstein wrote: > ext4_handle_release_buffer() is an API, which is not being used properly. > This is not so bad considering that it calls jbd2_journal_release_buffer(), > which does nothing, but the snapshots implementation of this API does > something. > > ext4_jbd2.h defines 2 identical wrapper functions: > ext4_journal_release_buffer() and ext4_handle_release_buffer(). > The former has no callers, so it was removed. Note that there are some commented-out calls to ext4_journal_release_buffer() in resize.c; they're probably commented out because at least today, jbd2_journal_release_buffer() is indeed a no-op. I'll update resize.c to use the preferred call. Speaking of which, how does on-line resize work in the face of the snapshot feature? Is that something that is or isn't supported? Note also that there may be places where a buffer has get_write_access() called on it, only for it to be never modified. resize.c is an example of that. > There are 2 users of the API: > ext4_new_inode() calls ext4_handle_release_buffer() and > ext4_xattr_block_set() calls jbd2_journal_release_buffer() directly. > The latter was chagned to call the wrapper API. > > Signed-off-by: Amir Goldstein <amir73il@users.sf.net> I'll apply this patch the changes. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 21, 2011 at 4:43 AM, Ted Ts'o <tytso@mit.edu> wrote: > On Tue, Mar 01, 2011 at 03:30:24PM +0200, Amir Goldstein wrote: >> ext4_handle_release_buffer() is an API, which is not being used properly. >> This is not so bad considering that it calls jbd2_journal_release_buffer(), >> which does nothing, but the snapshots implementation of this API does >> something. >> >> ext4_jbd2.h defines 2 identical wrapper functions: >> ext4_journal_release_buffer() and ext4_handle_release_buffer(). >> The former has no callers, so it was removed. > > Note that there are some commented-out calls to > ext4_journal_release_buffer() in resize.c; they're probably commented > out because at least today, jbd2_journal_release_buffer() is indeed a > no-op. > > I'll update resize.c to use the preferred call. Speaking of which, > how does on-line resize work in the face of the snapshot feature? Is > that something that is or isn't supported? Yes, it is supported. (offline resize isn't supported). The old snapshots sizes (i_disksize) represent the file system size at the time of the snapshot. > > Note also that there may be places where a buffer has > get_write_access() called on it, only for it to be never modified. > resize.c is an example of that. Those places can be a problem (since COW does happen and use up transaction credits) and calling buffer_release can help (it tries to extend the transaction), but specifically, resize.c is not a problem, since blocks outside the active snapshot's i_disksize are never COWed. > >> There are 2 users of the API: >> ext4_new_inode() calls ext4_handle_release_buffer() and >> ext4_xattr_block_set() calls jbd2_journal_release_buffer() directly. >> The latter was chagned to call the wrapper API. >> >> Signed-off-by: Amir Goldstein <amir73il@users.sf.net> > > I'll apply this patch the changes. > > - Ted > > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h index d8b992e..e25e99b 100644 --- a/fs/ext4/ext4_jbd2.h +++ b/fs/ext4/ext4_jbd2.h @@ -202,13 +202,6 @@ static inline int ext4_handle_has_enough_credits(handle_t *handle, int needed) return 1; } -static inline void ext4_journal_release_buffer(handle_t *handle, - struct buffer_head *bh) -{ - if (ext4_handle_valid(handle)) - jbd2_journal_release_buffer(handle, bh); -} - static inline handle_t *ext4_journal_start(struct inode *inode, int nblocks) { return ext4_journal_start_sb(inode->i_sb, nblocks); diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index f4c03af..b545ca1 100644 --- a/fs/ext4/xattr.c +++ b/fs/ext4/xattr.c @@ -735,7 +735,7 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode, int offset = (char *)s->here - bs->bh->b_data; unlock_buffer(bs->bh); - jbd2_journal_release_buffer(handle, bs->bh); + ext4_handle_release_buffer(handle, bs->bh); if (ce) { mb_cache_entry_release(ce); ce = NULL;
ext4_handle_release_buffer() is an API, which is not being used properly. This is not so bad considering that it calls jbd2_journal_release_buffer(), which does nothing, but the snapshots implementation of this API does something. ext4_jbd2.h defines 2 identical wrapper functions: ext4_journal_release_buffer() and ext4_handle_release_buffer(). The former has no callers, so it was removed. There are 2 users of the API: ext4_new_inode() calls ext4_handle_release_buffer() and ext4_xattr_block_set() calls jbd2_journal_release_buffer() directly. The latter was chagned to call the wrapper API. Signed-off-by: Amir Goldstein <amir73il@users.sf.net> --- fs/ext4/ext4_jbd2.h | 7 ------- fs/ext4/xattr.c | 2 +- 2 files changed, 1 insertions(+), 8 deletions(-)