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