* [PATCH 1/2] Use 'inode' variable that is already dereferenced @ 2012-10-15 19:06 Anatol Pomozov 2012-10-15 19:06 ` [PATCH 2/2] ext4: Remove code duplication in ext4_get_block_write_nolock() Anatol Pomozov 2012-10-16 2:28 ` [PATCH 1/2] Use 'inode' variable that is already dereferenced Zheng Liu 0 siblings, 2 replies; 9+ messages in thread From: Anatol Pomozov @ 2012-10-15 19:06 UTC (permalink / raw) To: tytso, wenqing.lz, linux-ext4; +Cc: Anatol Pomozov Tested: xfs tests Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com> --- fs/ext4/page-io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c index 68e896e..0fd16e6 100644 --- a/fs/ext4/page-io.c +++ b/fs/ext4/page-io.c @@ -111,7 +111,7 @@ static int ext4_end_io(ext4_io_end_t *io) inode_dio_done(inode); /* Wake up anyone waiting on unwritten extent conversion */ if (atomic_dec_and_test(&EXT4_I(inode)->i_unwritten)) - wake_up_all(ext4_ioend_wq(io->inode)); + wake_up_all(ext4_ioend_wq(inode)); return ret; } -- 1.8.0.rc1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] ext4: Remove code duplication in ext4_get_block_write_nolock() 2012-10-15 19:06 [PATCH 1/2] Use 'inode' variable that is already dereferenced Anatol Pomozov @ 2012-10-15 19:06 ` Anatol Pomozov 2012-10-15 19:14 ` Anatol Pomozov ` (3 more replies) 2012-10-16 2:28 ` [PATCH 1/2] Use 'inode' variable that is already dereferenced Zheng Liu 1 sibling, 4 replies; 9+ messages in thread From: Anatol Pomozov @ 2012-10-15 19:06 UTC (permalink / raw) To: tytso, wenqing.lz, linux-ext4; +Cc: Anatol Pomozov 729f52c6be51013 introduced function ext4_get_block_write_nolock() that is very similar to _ext4_get_block(). Eliminate code duplication by passing different flags to _ext4_get_block() Tested: xfs tests Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com> --- fs/ext4/inode.c | 63 ++++++++++++++++++++++----------------------------------- 1 file changed, 24 insertions(+), 39 deletions(-) diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index b3c243b..9829c3d 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -683,7 +683,7 @@ static int _ext4_get_block(struct inode *inode, sector_t iblock, map.m_lblk = iblock; map.m_len = bh->b_size >> inode->i_blkbits; - if (flags && !handle) { + if (flags && !(flags | EXT4_GET_BLOCKS_NO_LOCK) && !handle) { /* Direct IO write... */ if (map.m_len > DIO_MAX_BLOCKS) map.m_len = DIO_MAX_BLOCKS; @@ -880,6 +880,8 @@ static int do_journal_get_write_access(handle_t *handle, static int ext4_get_block_write(struct inode *inode, sector_t iblock, struct buffer_head *bh_result, int create); +static int ext4_get_block_write_nolock(struct inode *inode, sector_t iblock, + struct buffer_head *bh_result, int create); static int ext4_write_begin(struct file *file, struct address_space *mapping, loff_t pos, unsigned len, unsigned flags, struct page **pagep, void **fsdata) @@ -2850,29 +2852,12 @@ static int ext4_get_block_write(struct inode *inode, sector_t iblock, } static int ext4_get_block_write_nolock(struct inode *inode, sector_t iblock, - struct buffer_head *bh_result, int flags) + struct buffer_head *bh_result, int create) { - handle_t *handle = ext4_journal_current_handle(); - struct ext4_map_blocks map; - int ret = 0; - - ext4_debug("ext4_get_block_write_nolock: inode %lu, flag %d\n", - inode->i_ino, flags); - - flags = EXT4_GET_BLOCKS_NO_LOCK; - - map.m_lblk = iblock; - map.m_len = bh_result->b_size >> inode->i_blkbits; - - ret = ext4_map_blocks(handle, inode, &map, flags); - if (ret > 0) { - map_bh(bh_result, inode->i_sb, map.m_pblk); - bh_result->b_state = (bh_result->b_state & ~EXT4_MAP_FLAGS) | - map.m_flags; - bh_result->b_size = inode->i_sb->s_blocksize * map.m_len; - ret = 0; - } - return ret; + ext4_debug("ext4_get_block_write_nolock: inode %lu, create flag %d\n", + inode->i_ino, create); + return _ext4_get_block(inode, iblock, bh_result, + EXT4_GET_BLOCKS_NO_LOCK); } static void ext4_end_io_dio(struct kiocb *iocb, loff_t offset, @@ -3003,6 +2988,8 @@ static ssize_t ext4_ext_direct_IO(int rw, struct kiocb *iocb, loff_t final_size = offset + count; if (rw == WRITE && final_size <= inode->i_size) { int overwrite = 0; + get_block_t *get_block_func = NULL; + int dio_flags = 0; BUG_ON(iocb->private == NULL); @@ -3056,22 +3043,20 @@ static ssize_t ext4_ext_direct_IO(int rw, struct kiocb *iocb, ext4_inode_aio_set(inode, io_end); } - if (overwrite) - ret = __blockdev_direct_IO(rw, iocb, inode, - inode->i_sb->s_bdev, iov, - offset, nr_segs, - ext4_get_block_write_nolock, - ext4_end_io_dio, - NULL, - 0); - else - ret = __blockdev_direct_IO(rw, iocb, inode, - inode->i_sb->s_bdev, iov, - offset, nr_segs, - ext4_get_block_write, - ext4_end_io_dio, - NULL, - DIO_LOCKING); + if (overwrite) { + get_block_func = ext4_get_block_write_nolock; + } else { + get_block_func = ext4_get_block_write; + dio_flags = DIO_LOCKING; + } + ret = __blockdev_direct_IO(rw, iocb, inode, + inode->i_sb->s_bdev, iov, + offset, nr_segs, + get_block_func, + ext4_end_io_dio, + NULL, + dio_flags); + if (iocb->private) ext4_inode_aio_set(inode, NULL); /* -- 1.8.0.rc1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ext4: Remove code duplication in ext4_get_block_write_nolock() 2012-10-15 19:06 ` [PATCH 2/2] ext4: Remove code duplication in ext4_get_block_write_nolock() Anatol Pomozov @ 2012-10-15 19:14 ` Anatol Pomozov 2012-10-16 2:37 ` Zheng Liu 2012-10-16 2:36 ` Zheng Liu ` (2 subsequent siblings) 3 siblings, 1 reply; 9+ messages in thread From: Anatol Pomozov @ 2012-10-15 19:14 UTC (permalink / raw) To: tytso, wenqing.lz, linux-ext4; +Cc: Anatol Pomozov On Mon, Oct 15, 2012 at 12:06 PM, Anatol Pomozov <anatol.pomozov@gmail.com> wrote: > 729f52c6be51013 introduced function ext4_get_block_write_nolock() that > is very similar to _ext4_get_block(). Eliminate code duplication > by passing different flags to _ext4_get_block() > > Tested: xfs tests > > Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com> > --- > fs/ext4/inode.c | 63 ++++++++++++++++++++++----------------------------------- > 1 file changed, 24 insertions(+), 39 deletions(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index b3c243b..9829c3d 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -683,7 +683,7 @@ static int _ext4_get_block(struct inode *inode, sector_t iblock, > map.m_lblk = iblock; > map.m_len = bh->b_size >> inode->i_blkbits; > > - if (flags && !handle) { > + if (flags && !(flags | EXT4_GET_BLOCKS_NO_LOCK) && !handle) { > /* Direct IO write... */ > if (map.m_len > DIO_MAX_BLOCKS) > map.m_len = DIO_MAX_BLOCKS; > @@ -880,6 +880,8 @@ static int do_journal_get_write_access(handle_t *handle, > > static int ext4_get_block_write(struct inode *inode, sector_t iblock, > struct buffer_head *bh_result, int create); > +static int ext4_get_block_write_nolock(struct inode *inode, sector_t iblock, > + struct buffer_head *bh_result, int create); > static int ext4_write_begin(struct file *file, struct address_space *mapping, > loff_t pos, unsigned len, unsigned flags, > struct page **pagep, void **fsdata) > @@ -2850,29 +2852,12 @@ static int ext4_get_block_write(struct inode *inode, sector_t iblock, > } > > static int ext4_get_block_write_nolock(struct inode *inode, sector_t iblock, > - struct buffer_head *bh_result, int flags) > + struct buffer_head *bh_result, int create) > { > - handle_t *handle = ext4_journal_current_handle(); > - struct ext4_map_blocks map; > - int ret = 0; > - > - ext4_debug("ext4_get_block_write_nolock: inode %lu, flag %d\n", > - inode->i_ino, flags); > - > - flags = EXT4_GET_BLOCKS_NO_LOCK; > - > - map.m_lblk = iblock; > - map.m_len = bh_result->b_size >> inode->i_blkbits; > - > - ret = ext4_map_blocks(handle, inode, &map, flags); > - if (ret > 0) { > - map_bh(bh_result, inode->i_sb, map.m_pblk); > - bh_result->b_state = (bh_result->b_state & ~EXT4_MAP_FLAGS) | > - map.m_flags; > - bh_result->b_size = inode->i_sb->s_blocksize * map.m_len; > - ret = 0; > - } > - return ret; > + ext4_debug("ext4_get_block_write_nolock: inode %lu, create flag %d\n", > + inode->i_ino, create); > + return _ext4_get_block(inode, iblock, bh_result, > + EXT4_GET_BLOCKS_NO_LOCK); > } > > static void ext4_end_io_dio(struct kiocb *iocb, loff_t offset, > @@ -3003,6 +2988,8 @@ static ssize_t ext4_ext_direct_IO(int rw, struct kiocb *iocb, > loff_t final_size = offset + count; > if (rw == WRITE && final_size <= inode->i_size) { > int overwrite = 0; > + get_block_t *get_block_func = NULL; > + int dio_flags = 0; > > BUG_ON(iocb->private == NULL); > > @@ -3056,22 +3043,20 @@ static ssize_t ext4_ext_direct_IO(int rw, struct kiocb *iocb, > ext4_inode_aio_set(inode, io_end); > } > > - if (overwrite) > - ret = __blockdev_direct_IO(rw, iocb, inode, > - inode->i_sb->s_bdev, iov, > - offset, nr_segs, > - ext4_get_block_write_nolock, > - ext4_end_io_dio, > - NULL, > - 0); > - else > - ret = __blockdev_direct_IO(rw, iocb, inode, > - inode->i_sb->s_bdev, iov, > - offset, nr_segs, > - ext4_get_block_write, > - ext4_end_io_dio, > - NULL, > - DIO_LOCKING); > + if (overwrite) { > + get_block_func = ext4_get_block_write_nolock; Ideally if block layer (function __blockdev_direct_IO) accepted some user-data (e.g. flag) and later passed it back to callback function (i.e. ext4_get_block_write). In this case we do not need two functions ext4_get_block_write/ext4_get_block_write_nolock. > + } else { > + get_block_func = ext4_get_block_write; > + dio_flags = DIO_LOCKING; > + } > + ret = __blockdev_direct_IO(rw, iocb, inode, > + inode->i_sb->s_bdev, iov, > + offset, nr_segs, > + get_block_func, > + ext4_end_io_dio, > + NULL, > + dio_flags); > + > if (iocb->private) > ext4_inode_aio_set(inode, NULL); > /* > -- > 1.8.0.rc1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ext4: Remove code duplication in ext4_get_block_write_nolock() 2012-10-15 19:14 ` Anatol Pomozov @ 2012-10-16 2:37 ` Zheng Liu 0 siblings, 0 replies; 9+ messages in thread From: Zheng Liu @ 2012-10-16 2:37 UTC (permalink / raw) To: Anatol Pomozov; +Cc: tytso, wenqing.lz, linux-ext4 On Mon, Oct 15, 2012 at 12:14:37PM -0700, Anatol Pomozov wrote: [snip] > Ideally if block layer (function __blockdev_direct_IO) accepted some > user-data (e.g. flag) and later passed it back to callback function > (i.e. ext4_get_block_write). In this case we do not need two functions > ext4_get_block_write/ext4_get_block_write_nolock. Thanks for teaching me. :-) Regards, Zheng ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ext4: Remove code duplication in ext4_get_block_write_nolock() 2012-10-15 19:06 ` [PATCH 2/2] ext4: Remove code duplication in ext4_get_block_write_nolock() Anatol Pomozov 2012-10-15 19:14 ` Anatol Pomozov @ 2012-10-16 2:36 ` Zheng Liu 2012-11-08 20:08 ` Theodore Ts'o 2012-11-09 17:29 ` Theodore Ts'o 3 siblings, 0 replies; 9+ messages in thread From: Zheng Liu @ 2012-10-16 2:36 UTC (permalink / raw) To: Anatol Pomozov; +Cc: tytso, wenqing.lz, linux-ext4 On Mon, Oct 15, 2012 at 12:06:49PM -0700, Anatol Pomozov wrote: > 729f52c6be51013 introduced function ext4_get_block_write_nolock() that > is very similar to _ext4_get_block(). Eliminate code duplication > by passing different flags to _ext4_get_block() > > Tested: xfs tests > > Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com> Good catch. you can add: Reviewed-by: Zheng Liu <wenqing.lz@taobao.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ext4: Remove code duplication in ext4_get_block_write_nolock() 2012-10-15 19:06 ` [PATCH 2/2] ext4: Remove code duplication in ext4_get_block_write_nolock() Anatol Pomozov 2012-10-15 19:14 ` Anatol Pomozov 2012-10-16 2:36 ` Zheng Liu @ 2012-11-08 20:08 ` Theodore Ts'o 2012-11-09 17:29 ` Theodore Ts'o 3 siblings, 0 replies; 9+ messages in thread From: Theodore Ts'o @ 2012-11-08 20:08 UTC (permalink / raw) To: Anatol Pomozov; +Cc: wenqing.lz, linux-ext4 On Mon, Oct 15, 2012 at 12:06:49PM -0700, Anatol Pomozov wrote: > 729f52c6be51013 introduced function ext4_get_block_write_nolock() that > is very similar to _ext4_get_block(). Eliminate code duplication > by passing different flags to _ext4_get_block() > > Tested: xfs tests > > Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com> Applied, thanks. - Ted ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ext4: Remove code duplication in ext4_get_block_write_nolock() 2012-10-15 19:06 ` [PATCH 2/2] ext4: Remove code duplication in ext4_get_block_write_nolock() Anatol Pomozov ` (2 preceding siblings ...) 2012-11-08 20:08 ` Theodore Ts'o @ 2012-11-09 17:29 ` Theodore Ts'o 2012-11-09 17:51 ` Anatol Pomozov 3 siblings, 1 reply; 9+ messages in thread From: Theodore Ts'o @ 2012-11-09 17:29 UTC (permalink / raw) To: Anatol Pomozov; +Cc: wenqing.lz, linux-ext4 There's a really serious bug in this patch which I didn't notice at first: > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -683,7 +683,7 @@ static int _ext4_get_block(struct inode *inode, sector_t iblock, > map.m_lblk = iblock; > map.m_len = bh->b_size >> inode->i_blkbits; > > - if (flags && !handle) { > + if (flags && !(flags | EXT4_GET_BLOCKS_NO_LOCK) && !handle) { ^^^ This should obviously read: if (flags && !(flags & EXT4_GET_BLOCKS_NO_LOCK) && !handle) { Otherwise xfstests #91 will blow out with a circular lockdep warning. As a reminder, please do make sure you compile with lockdep enabled when you run your tests; it's found more than one bug for me! - Ted ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ext4: Remove code duplication in ext4_get_block_write_nolock() 2012-11-09 17:29 ` Theodore Ts'o @ 2012-11-09 17:51 ` Anatol Pomozov 0 siblings, 0 replies; 9+ messages in thread From: Anatol Pomozov @ 2012-11-09 17:51 UTC (permalink / raw) To: Theodore Ts'o; +Cc: wenqing.lz, linux-ext4 Hi On Fri, Nov 9, 2012 at 9:29 AM, Theodore Ts'o <tytso@mit.edu> wrote: > There's a really serious bug in this patch which I didn't notice at > first: > >> --- a/fs/ext4/inode.c >> +++ b/fs/ext4/inode.c >> @@ -683,7 +683,7 @@ static int _ext4_get_block(struct inode *inode, sector_t iblock, >> map.m_lblk = iblock; >> map.m_len = bh->b_size >> inode->i_blkbits; >> >> - if (flags && !handle) { >> + if (flags && !(flags | EXT4_GET_BLOCKS_NO_LOCK) && !handle) { > ^^^ > > This should obviously read: > > if (flags && !(flags & EXT4_GET_BLOCKS_NO_LOCK) && !handle) { D'oh... You are right.. > Otherwise xfstests #91 will blow out with a circular lockdep warning. > > As a reminder, please do make sure you compile with lockdep enabled > when you run your tests; it's found more than one bug for me! I though I tested with and without debug options enabled. Maybe I missed lockdep option somehow, next time i'll make sure that it is enabled. Thanks for catching it. > > - Ted ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Use 'inode' variable that is already dereferenced 2012-10-15 19:06 [PATCH 1/2] Use 'inode' variable that is already dereferenced Anatol Pomozov 2012-10-15 19:06 ` [PATCH 2/2] ext4: Remove code duplication in ext4_get_block_write_nolock() Anatol Pomozov @ 2012-10-16 2:28 ` Zheng Liu 1 sibling, 0 replies; 9+ messages in thread From: Zheng Liu @ 2012-10-16 2:28 UTC (permalink / raw) To: Anatol Pomozov; +Cc: tytso, wenqing.lz, linux-ext4 On Mon, Oct 15, 2012 at 12:06:48PM -0700, Anatol Pomozov wrote: > Tested: xfs tests > > Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com> > --- > fs/ext4/page-io.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c > index 68e896e..0fd16e6 100644 > --- a/fs/ext4/page-io.c > +++ b/fs/ext4/page-io.c > @@ -111,7 +111,7 @@ static int ext4_end_io(ext4_io_end_t *io) > inode_dio_done(inode); > /* Wake up anyone waiting on unwritten extent conversion */ > if (atomic_dec_and_test(&EXT4_I(inode)->i_unwritten)) > - wake_up_all(ext4_ioend_wq(io->inode)); > + wake_up_all(ext4_ioend_wq(inode)); Reviewed-by: Zheng Liu <wenqing.lz@taobao.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-11-09 17:51 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-10-15 19:06 [PATCH 1/2] Use 'inode' variable that is already dereferenced Anatol Pomozov 2012-10-15 19:06 ` [PATCH 2/2] ext4: Remove code duplication in ext4_get_block_write_nolock() Anatol Pomozov 2012-10-15 19:14 ` Anatol Pomozov 2012-10-16 2:37 ` Zheng Liu 2012-10-16 2:36 ` Zheng Liu 2012-11-08 20:08 ` Theodore Ts'o 2012-11-09 17:29 ` Theodore Ts'o 2012-11-09 17:51 ` Anatol Pomozov 2012-10-16 2:28 ` [PATCH 1/2] Use 'inode' variable that is already dereferenced Zheng Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).