* [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 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
* 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: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: 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
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).