* [PATCH 1/2] ext4: quota_write cross block boundary behaviour
@ 2010-02-16 16:33 Dmitry Monakhov
2010-02-16 16:33 ` [PATCH 2/2] ext3: " Dmitry Monakhov
2010-02-16 18:46 ` [PATCH 1/2] ext4: " Jan Kara
0 siblings, 2 replies; 4+ messages in thread
From: Dmitry Monakhov @ 2010-02-16 16:33 UTC (permalink / raw)
To: linux-ext4; +Cc: jack, Dmitry Monakhov
We always assume what dquot update result in changes in one data block
But ext4_quota_write() function may handle cross block boundary writes
In fact if this ever happen it will result in incorrect journal credits
reservation. And later bug_on triggering. As soon this never happen the
boundary cross loop is NOOP. In order to make things straight
let's remove this loop and assert cross boundary condition.
Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
fs/ext4/super.c | 69 +++++++++++++++++++++++++++----------------------------
1 files changed, 34 insertions(+), 35 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 9e45e62..d5596ca 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3940,9 +3940,7 @@ static ssize_t ext4_quota_write(struct super_block *sb, int type,
ext4_lblk_t blk = off >> EXT4_BLOCK_SIZE_BITS(sb);
int err = 0;
int offset = off & (sb->s_blocksize - 1);
- int tocopy;
int journal_quota = EXT4_SB(sb)->s_qf_names[type] != NULL;
- size_t towrite = len;
struct buffer_head *bh;
handle_t *handle = journal_current_handle();
@@ -3952,52 +3950,53 @@ static ssize_t ext4_quota_write(struct super_block *sb, int type,
(unsigned long long)off, (unsigned long long)len);
return -EIO;
}
+ /*
+ * Since we account only one data block in transaction credits,
+ * then it is impossible to cross a block boundary.
+ */
+ if (sb->s_blocksize - offset < len) {
+ ext4_msg(sb, KERN_WARNING, "Quota write (off=%llu, len=%llu)"
+ " cancelled because not block aligned",
+ (unsigned long long)off, (unsigned long long)len);
+ return -EIO;
+ }
+
mutex_lock_nested(&inode->i_mutex, I_MUTEX_QUOTA);
- while (towrite > 0) {
- tocopy = sb->s_blocksize - offset < towrite ?
- sb->s_blocksize - offset : towrite;
- bh = ext4_bread(handle, inode, blk, 1, &err);
- if (!bh)
+ bh = ext4_bread(handle, inode, blk, 1, &err);
+ if (!bh)
+ goto out;
+ if (journal_quota) {
+ err = ext4_journal_get_write_access(handle, bh);
+ if (err) {
+ brelse(bh);
goto out;
- if (journal_quota) {
- err = ext4_journal_get_write_access(handle, bh);
- if (err) {
- brelse(bh);
- goto out;
- }
}
- lock_buffer(bh);
- memcpy(bh->b_data+offset, data, tocopy);
- flush_dcache_page(bh->b_page);
- unlock_buffer(bh);
- if (journal_quota)
- err = ext4_handle_dirty_metadata(handle, NULL, bh);
- else {
- /* Always do at least ordered writes for quotas */
- err = ext4_jbd2_file_inode(handle, inode);
- mark_buffer_dirty(bh);
- }
- brelse(bh);
- if (err)
- goto out;
- offset = 0;
- towrite -= tocopy;
- data += tocopy;
- blk++;
}
+ lock_buffer(bh);
+ memcpy(bh->b_data+offset, data, len);
+ flush_dcache_page(bh->b_page);
+ unlock_buffer(bh);
+ if (journal_quota)
+ err = ext4_handle_dirty_metadata(handle, NULL, bh);
+ else {
+ /* Always do at least ordered writes for quotas */
+ err = ext4_jbd2_file_inode(handle, inode);
+ mark_buffer_dirty(bh);
+ }
+ brelse(bh);
out:
- if (len == towrite) {
+ if (err) {
mutex_unlock(&inode->i_mutex);
return err;
}
- if (inode->i_size < off+len-towrite) {
- i_size_write(inode, off+len-towrite);
+ if (inode->i_size < off + len) {
+ i_size_write(inode, off + len);
EXT4_I(inode)->i_disksize = inode->i_size;
}
inode->i_mtime = inode->i_ctime = CURRENT_TIME;
ext4_mark_inode_dirty(handle, inode);
mutex_unlock(&inode->i_mutex);
- return len - towrite;
+ return len;
}
#endif
--
1.6.6
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] ext3: quota_write cross block boundary behaviour
2010-02-16 16:33 [PATCH 1/2] ext4: quota_write cross block boundary behaviour Dmitry Monakhov
@ 2010-02-16 16:33 ` Dmitry Monakhov
2010-02-16 18:46 ` [PATCH 1/2] ext4: " Jan Kara
1 sibling, 0 replies; 4+ messages in thread
From: Dmitry Monakhov @ 2010-02-16 16:33 UTC (permalink / raw)
To: linux-ext4; +Cc: jack, Dmitry Monakhov
We always assume what dquot update result in changes in one data block
But ext3_quota_write() function may handle cross block boundary writes
In fact if this ever happen it will result in incorrect journal credits
reservation. And later bug_on triggering. As soon this never happen the
boundary cross loop is NOOP. In order to make things straight
let's remove this loop and assert cross boundary condition.
Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
fs/ext3/super.c | 69 +++++++++++++++++++++++++++----------------------------
1 files changed, 34 insertions(+), 35 deletions(-)
diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index afa2b56..016c841 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -2942,9 +2942,7 @@ static ssize_t ext3_quota_write(struct super_block *sb, int type,
sector_t blk = off >> EXT3_BLOCK_SIZE_BITS(sb);
int err = 0;
int offset = off & (sb->s_blocksize - 1);
- int tocopy;
int journal_quota = EXT3_SB(sb)->s_qf_names[type] != NULL;
- size_t towrite = len;
struct buffer_head *bh;
handle_t *handle = journal_current_handle();
@@ -2955,53 +2953,54 @@ static ssize_t ext3_quota_write(struct super_block *sb, int type,
(unsigned long long)off, (unsigned long long)len);
return -EIO;
}
+
+ /*
+ * Since we account only one data block in transaction credits,
+ * then it is impossible to cross a block boundary.
+ */
+ if (sb->s_blocksize - offset < len) {
+ ext3_msg(sb, KERN_WARNING, "Quota write (off=%llu, len=%llu)"
+ " cancelled because not block aligned",
+ (unsigned long long)off, (unsigned long long)len);
+ return -EIO;
+ }
mutex_lock_nested(&inode->i_mutex, I_MUTEX_QUOTA);
- while (towrite > 0) {
- tocopy = sb->s_blocksize - offset < towrite ?
- sb->s_blocksize - offset : towrite;
- bh = ext3_bread(handle, inode, blk, 1, &err);
- if (!bh)
+ bh = ext3_bread(handle, inode, blk, 1, &err);
+ if (!bh)
+ goto out;
+ if (journal_quota) {
+ err = ext3_journal_get_write_access(handle, bh);
+ if (err) {
+ brelse(bh);
goto out;
- if (journal_quota) {
- err = ext3_journal_get_write_access(handle, bh);
- if (err) {
- brelse(bh);
- goto out;
- }
- }
- lock_buffer(bh);
- memcpy(bh->b_data+offset, data, tocopy);
- flush_dcache_page(bh->b_page);
- unlock_buffer(bh);
- if (journal_quota)
- err = ext3_journal_dirty_metadata(handle, bh);
- else {
- /* Always do at least ordered writes for quotas */
- err = ext3_journal_dirty_data(handle, bh);
- mark_buffer_dirty(bh);
}
- brelse(bh);
- if (err)
- goto out;
- offset = 0;
- towrite -= tocopy;
- data += tocopy;
- blk++;
}
+ lock_buffer(bh);
+ memcpy(bh->b_data+offset, data, len);
+ flush_dcache_page(bh->b_page);
+ unlock_buffer(bh);
+ if (journal_quota)
+ err = ext3_journal_dirty_metadata(handle, bh);
+ else {
+ /* Always do at least ordered writes for quotas */
+ err = ext3_journal_dirty_data(handle, bh);
+ mark_buffer_dirty(bh);
+ }
+ brelse(bh);
out:
- if (len == towrite) {
+ if (err) {
mutex_unlock(&inode->i_mutex);
return err;
}
- if (inode->i_size < off+len-towrite) {
- i_size_write(inode, off+len-towrite);
+ if (inode->i_size < off + len) {
+ i_size_write(inode, off + len);
EXT3_I(inode)->i_disksize = inode->i_size;
}
inode->i_version++;
inode->i_mtime = inode->i_ctime = CURRENT_TIME;
ext3_mark_inode_dirty(handle, inode);
mutex_unlock(&inode->i_mutex);
- return len - towrite;
+ return len;
}
#endif
--
1.6.6
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] ext4: quota_write cross block boundary behaviour
2010-02-16 16:33 [PATCH 1/2] ext4: quota_write cross block boundary behaviour Dmitry Monakhov
2010-02-16 16:33 ` [PATCH 2/2] ext3: " Dmitry Monakhov
@ 2010-02-16 18:46 ` Jan Kara
[not found] ` <878wab6p3s.fsf@openvz.org>
1 sibling, 1 reply; 4+ messages in thread
From: Jan Kara @ 2010-02-16 18:46 UTC (permalink / raw)
To: Dmitry Monakhov; +Cc: linux-ext4, jack, tytso
On Tue 16-02-10 19:33:41, Dmitry Monakhov wrote:
> We always assume what dquot update result in changes in one data block
> But ext4_quota_write() function may handle cross block boundary writes
> In fact if this ever happen it will result in incorrect journal credits
> reservation. And later bug_on triggering. As soon this never happen the
> boundary cross loop is NOOP. In order to make things straight
> let's remove this loop and assert cross boundary condition.
>
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
Yeah, originally I thought it might be useful to support a possibility of
multiblock writes but in the end we never needed it and currently e.g. OCFS2
would already BUG on that so yes, this is a good simplification.
Acked-by: Jan Kara <jack@suse.cz>
I've merged the ext3 version of the patch into my tree. Ted, will you
merge this ext4 cleanup please?
Honza
> ---
> fs/ext4/super.c | 69 +++++++++++++++++++++++++++----------------------------
> 1 files changed, 34 insertions(+), 35 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 9e45e62..d5596ca 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3940,9 +3940,7 @@ static ssize_t ext4_quota_write(struct super_block *sb, int type,
> ext4_lblk_t blk = off >> EXT4_BLOCK_SIZE_BITS(sb);
> int err = 0;
> int offset = off & (sb->s_blocksize - 1);
> - int tocopy;
> int journal_quota = EXT4_SB(sb)->s_qf_names[type] != NULL;
> - size_t towrite = len;
> struct buffer_head *bh;
> handle_t *handle = journal_current_handle();
>
> @@ -3952,52 +3950,53 @@ static ssize_t ext4_quota_write(struct super_block *sb, int type,
> (unsigned long long)off, (unsigned long long)len);
> return -EIO;
> }
> + /*
> + * Since we account only one data block in transaction credits,
> + * then it is impossible to cross a block boundary.
> + */
> + if (sb->s_blocksize - offset < len) {
> + ext4_msg(sb, KERN_WARNING, "Quota write (off=%llu, len=%llu)"
> + " cancelled because not block aligned",
> + (unsigned long long)off, (unsigned long long)len);
> + return -EIO;
> + }
> +
> mutex_lock_nested(&inode->i_mutex, I_MUTEX_QUOTA);
> - while (towrite > 0) {
> - tocopy = sb->s_blocksize - offset < towrite ?
> - sb->s_blocksize - offset : towrite;
> - bh = ext4_bread(handle, inode, blk, 1, &err);
> - if (!bh)
> + bh = ext4_bread(handle, inode, blk, 1, &err);
> + if (!bh)
> + goto out;
> + if (journal_quota) {
> + err = ext4_journal_get_write_access(handle, bh);
> + if (err) {
> + brelse(bh);
> goto out;
> - if (journal_quota) {
> - err = ext4_journal_get_write_access(handle, bh);
> - if (err) {
> - brelse(bh);
> - goto out;
> - }
> }
> - lock_buffer(bh);
> - memcpy(bh->b_data+offset, data, tocopy);
> - flush_dcache_page(bh->b_page);
> - unlock_buffer(bh);
> - if (journal_quota)
> - err = ext4_handle_dirty_metadata(handle, NULL, bh);
> - else {
> - /* Always do at least ordered writes for quotas */
> - err = ext4_jbd2_file_inode(handle, inode);
> - mark_buffer_dirty(bh);
> - }
> - brelse(bh);
> - if (err)
> - goto out;
> - offset = 0;
> - towrite -= tocopy;
> - data += tocopy;
> - blk++;
> }
> + lock_buffer(bh);
> + memcpy(bh->b_data+offset, data, len);
> + flush_dcache_page(bh->b_page);
> + unlock_buffer(bh);
> + if (journal_quota)
> + err = ext4_handle_dirty_metadata(handle, NULL, bh);
> + else {
> + /* Always do at least ordered writes for quotas */
> + err = ext4_jbd2_file_inode(handle, inode);
> + mark_buffer_dirty(bh);
> + }
> + brelse(bh);
> out:
> - if (len == towrite) {
> + if (err) {
> mutex_unlock(&inode->i_mutex);
> return err;
> }
> - if (inode->i_size < off+len-towrite) {
> - i_size_write(inode, off+len-towrite);
> + if (inode->i_size < off + len) {
> + i_size_write(inode, off + len);
> EXT4_I(inode)->i_disksize = inode->i_size;
> }
> inode->i_mtime = inode->i_ctime = CURRENT_TIME;
> ext4_mark_inode_dirty(handle, inode);
> mutex_unlock(&inode->i_mutex);
> - return len - towrite;
> + return len;
> }
>
> #endif
> --
> 1.6.6
>
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] ext4: quota_write cross block boundary behaviour
[not found] ` <878wab6p3s.fsf@openvz.org>
@ 2010-03-02 13:15 ` tytso
0 siblings, 0 replies; 4+ messages in thread
From: tytso @ 2010-03-02 13:15 UTC (permalink / raw)
To: Dmitry Monakhov; +Cc: linux-ext4
On Tue, Mar 02, 2010 at 12:37:43PM +0300, Dmitry Monakhov wrote:
> Jan Kara <jack@suse.cz> writes:
>
> > On Tue 16-02-10 19:33:41, Dmitry Monakhov wrote:
> >> We always assume what dquot update result in changes in one data block
> >> But ext4_quota_write() function may handle cross block boundary writes
> >> In fact if this ever happen it will result in incorrect journal credits
> >> reservation. And later bug_on triggering. As soon this never happen the
> >> boundary cross loop is NOOP. In order to make things straight
> >> let's remove this loop and assert cross boundary condition.
> >>
> >> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> > Yeah, originally I thought it might be useful to support a possibility of
> > multiblock writes but in the end we never needed it and currently e.g. OCFS2
> > would already BUG on that so yes, this is a good simplification.
> > Acked-by: Jan Kara <jack@suse.cz>
> >
> > I've merged the ext3 version of the patch into my tree. Ted, will you
> > merge this ext4 cleanup please?
> Ted please take a look at the patch.
Sorry, I had lost track of this patch. I've added it to the ext4 patch queue.
- Ted
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-03-02 13:15 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-02-16 16:33 [PATCH 1/2] ext4: quota_write cross block boundary behaviour Dmitry Monakhov
2010-02-16 16:33 ` [PATCH 2/2] ext3: " Dmitry Monakhov
2010-02-16 18:46 ` [PATCH 1/2] ext4: " Jan Kara
[not found] ` <878wab6p3s.fsf@openvz.org>
2010-03-02 13:15 ` tytso
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.