[PATCH] direct-io: Fix unsigned comparison overflow

[PATCH] direct-io: Fix unsigned comparison overflow

[Harish Kasiviswanathan]

The following issue was noticed while running some Peer-2-Peer
benchmarks on NVMe SSD (EXT4 fs).

The first write after file create is very slow. The cause for this
is that for the first time the code doesn't take the direct P2P path and
instead falls back to software copy. The function get_more_block() sets
'create' to 0 after comparing 'unsigned long fs_startblk = 0' with 'long
long (i_size_read(dio->inode) - 1) >> i_blkbits = 0x0xfffffffffffff'.

I understand this previous change 'direct-io: fix direct write stale
data exposure from concurrent buffered read' was introduced to handle a
corner case for sparse file. I am not too familiar with sparse file.
Please let me know if this change would break it

Harish Kasiviswanathan (1):
  direct-io: Fix unsigned comparison overflow

 fs/direct-io.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH] direct-io: Fix unsigned comparison overflow

[Harish Kasiviswanathan]

The first write after file create fails to take the direct IO
(Peer-to-Peer) path and falls back to slower software copy. The function
get_more_block() sets 'create' to 0 after comparing 'unsigned long
fs_startblk = 0' with 'long long (i_size_read(dio->inode) - 1) >>
i_blkbits = 0xfffffffffffff'.

Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>
---
 fs/direct-io.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 3aafb33..588aa17 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -687,8 +687,8 @@ static int get_more_blocks(struct dio *dio, struct dio_submit *sdio,
 		 */
 		create = dio->op == REQ_OP_WRITE;
 		if (dio->flags & DIO_SKIP_HOLES) {
-			if (fs_startblk <= ((i_size_read(dio->inode) - 1) >>
-							i_blkbits))
+			if ((loff_t)fs_startblk <=
+				((i_size_read(dio->inode) - 1) >> i_blkbits))
 				create = 0;
 		}
 
-- 
2.7.4

Re: [PATCH] direct-io: Fix unsigned comparison overflow

[Eryu Guan]

On Tue, Dec 05, 2017 at 10:57:53AM -0500, Harish Kasiviswanathan wrote:
> The first write after file create fails to take the direct IO
> (Peer-to-Peer) path and falls back to slower software copy. The function
> get_more_block() sets 'create' to 0 after comparing 'unsigned long
> fs_startblk = 0' with 'long long (i_size_read(dio->inode) - 1) >>
> i_blkbits = 0xfffffffffffff'.
> 
> Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>
> ---
>  fs/direct-io.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 3aafb33..588aa17 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -687,8 +687,8 @@ static int get_more_blocks(struct dio *dio, struct dio_submit *sdio,
>  		 */
>  		create = dio->op == REQ_OP_WRITE;
>  		if (dio->flags & DIO_SKIP_HOLES) {

I think the major problem here is that the original code didn't consider
the i_size=0 case (my bad). Perhaps we can make sure i_size is greater
than 0 first? As what we want to avoid is "fill holes inside i_size",
and i_size == 0 means we're guaranteed not to allocate new blocks inside
i_size.

Thanks,
Eryu

> -			if (fs_startblk <= ((i_size_read(dio->inode) - 1) >>
> -							i_blkbits))
> +			if ((loff_t)fs_startblk <=
> +				((i_size_read(dio->inode) - 1) >> i_blkbits))
>  				create = 0;
>  		}
>  
> -- 
> 2.7.4
>