* re: ext4: fold ext4_generic_write_end() into ext4_write_end()
@ 2013-04-02 8:11 Dan Carpenter
2013-04-02 11:09 ` Zheng Liu
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2013-04-02 8:11 UTC (permalink / raw)
To: wenqing.lz; +Cc: linux-ext4, kbuild
Hello Zheng Liu,
The patch 31e4045fda81: "ext4: fold ext4_generic_write_end() into
ext4_write_end()" from Mar 26, 2013, leads to the following warning:
"fs/ext4/inode.c:1169 ext4_write_end()
warn: unsigned 'copied' is never less than zero."
1110 static int ext4_write_end(struct file *file,
1111 struct address_space *mapping,
1112 loff_t pos, unsigned len, unsigned copied,
copied is unsigned.
1113 struct page *page, void *fsdata)
1114 {
1115 handle_t *handle = ext4_journal_current_handle();
1116 struct inode *inode = mapping->host;
1117 int ret = 0, ret2;
1118 int i_size_changed = 0;
1119
1120 trace_ext4_write_end(inode, pos, len, copied);
1121 if (ext4_test_inode_state(inode, EXT4_STATE_ORDERED_MODE)) {
1122 ret = ext4_jbd2_file_inode(handle, inode);
1123 if (ret) {
1124 unlock_page(page);
1125 page_cache_release(page);
1126 goto errout;
1127 }
1128 }
1129
1130 if (ext4_has_inline_data(inode))
1131 copied = ext4_write_inline_data_end(inode, pos, len,
1132 copied, page);
1133 else
1134 copied = block_write_end(file, mapping, pos,
1135 len, copied, page, fsdata);
I don't think these functions return negative error codes.
1136
1137 /*
1138 * No need to use i_size_read() here, the i_size
1139 * cannot change under us because we hole i_mutex.
1140 *
1141 * But it's important to update i_size while still holding page lock:
1142 * page writeout could otherwise come in and zero beyond i_size.
1143 */
1144 if (pos + copied > inode->i_size) {
1145 i_size_write(inode, pos + copied);
1146 i_size_changed = 1;
1147 }
1148
1149 if (pos + copied > EXT4_I(inode)->i_disksize) {
1150 /* We need to mark inode dirty even if
1151 * new_i_size is less that inode->i_size
1152 * but greater than i_disksize. (hint delalloc)
1153 */
1154 ext4_update_i_disksize(inode, (pos + copied));
1155 i_size_changed = 1;
1156 }
1157 unlock_page(page);
1158 page_cache_release(page);
1159
1160 /*
1161 * Don't mark the inode dirty under page lock. First, it unnecessarily
1162 * makes the holding time of page lock longer. Second, it forces lock
1163 * ordering of page lock and transaction start for journaling
1164 * filesystems.
1165 */
1166 if (i_size_changed)
1167 ext4_mark_inode_dirty(handle, inode);
1168
1169 if (copied < 0)
1170 ret = copied;
copied is never less than zero because it's unsigned.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: ext4: fold ext4_generic_write_end() into ext4_write_end()
2013-04-02 8:11 ext4: fold ext4_generic_write_end() into ext4_write_end() Dan Carpenter
@ 2013-04-02 11:09 ` Zheng Liu
0 siblings, 0 replies; 2+ messages in thread
From: Zheng Liu @ 2013-04-02 11:09 UTC (permalink / raw)
To: Dan Carpenter; +Cc: wenqing.lz, linux-ext4, kbuild, Theodore Ts'o
[Add Ted into cc list]
Hi Dan,
Thanks for pointing it out.
On Tue, Apr 02, 2013 at 11:11:44AM +0300, Dan Carpenter wrote:
> Hello Zheng Liu,
>
> The patch 31e4045fda81: "ext4: fold ext4_generic_write_end() into
> ext4_write_end()" from Mar 26, 2013, leads to the following warning:
> "fs/ext4/inode.c:1169 ext4_write_end()
> warn: unsigned 'copied' is never less than zero."
>
>
> 1110 static int ext4_write_end(struct file *file,
> 1111 struct address_space *mapping,
> 1112 loff_t pos, unsigned len, unsigned copied,
>
> copied is unsigned.
>
> 1113 struct page *page, void *fsdata)
> 1114 {
> 1115 handle_t *handle = ext4_journal_current_handle();
> 1116 struct inode *inode = mapping->host;
> 1117 int ret = 0, ret2;
> 1118 int i_size_changed = 0;
> 1119
> 1120 trace_ext4_write_end(inode, pos, len, copied);
> 1121 if (ext4_test_inode_state(inode, EXT4_STATE_ORDERED_MODE)) {
> 1122 ret = ext4_jbd2_file_inode(handle, inode);
> 1123 if (ret) {
> 1124 unlock_page(page);
> 1125 page_cache_release(page);
> 1126 goto errout;
> 1127 }
> 1128 }
> 1129
> 1130 if (ext4_has_inline_data(inode))
> 1131 copied = ext4_write_inline_data_end(inode, pos, len,
> 1132 copied, page);
> 1133 else
> 1134 copied = block_write_end(file, mapping, pos,
> 1135 len, copied, page, fsdata);
>
> I don't think these functions return negative error codes.
Yes, I check these two functions and they don't return a negative error
code.
>
> 1136
> 1137 /*
> 1138 * No need to use i_size_read() here, the i_size
> 1139 * cannot change under us because we hole i_mutex.
> 1140 *
> 1141 * But it's important to update i_size while still holding page lock:
> 1142 * page writeout could otherwise come in and zero beyond i_size.
> 1143 */
> 1144 if (pos + copied > inode->i_size) {
> 1145 i_size_write(inode, pos + copied);
> 1146 i_size_changed = 1;
> 1147 }
> 1148
> 1149 if (pos + copied > EXT4_I(inode)->i_disksize) {
> 1150 /* We need to mark inode dirty even if
> 1151 * new_i_size is less that inode->i_size
> 1152 * but greater than i_disksize. (hint delalloc)
> 1153 */
> 1154 ext4_update_i_disksize(inode, (pos + copied));
> 1155 i_size_changed = 1;
> 1156 }
> 1157 unlock_page(page);
> 1158 page_cache_release(page);
> 1159
> 1160 /*
> 1161 * Don't mark the inode dirty under page lock. First, it unnecessarily
> 1162 * makes the holding time of page lock longer. Second, it forces lock
> 1163 * ordering of page lock and transaction start for journaling
> 1164 * filesystems.
> 1165 */
> 1166 if (i_size_changed)
> 1167 ext4_mark_inode_dirty(handle, inode);
> 1168
> 1169 if (copied < 0)
> 1170 ret = copied;
>
> copied is never less than zero because it's unsigned.
Yes, it should be removed.
Ted, please remove this check.
Thanks,
- Zheng
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2013-04-02 10:53 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-02 8:11 ext4: fold ext4_generic_write_end() into ext4_write_end() Dan Carpenter
2013-04-02 11:09 ` 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).