* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.