Skip to contentSkip to conversation

fallocate(2) related cleanups

PATCH NAK Rohan Puri 2 participants 11 messages Jul 18, 2014 1 message not in the archive

Subsystems (3)
VFS ext4 file system Ceph client
lore
Share & tools
lore

Rohan Puri

author
#0 lore raw

cover fallocate(2) related cleanups

This patch series contains fallocate(2) related cleanups

Rohan Puri (2):
Remove misleading comment in do_fallocate() Remove redundant inode mode checks for ceph & ext4

 fs/ceph/file.c  | 3 ---
 fs/ext4/inode.c | 3 ---
 fs/open.c       | 4 ----
 3 files changed, 10 deletions(-)
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH 1/2 Remove misleading comment in do_fallocate()

Preallocation for directory gets blocked at 2 places & never gets propogated to the underlying file system : -

- open(2) with O_DIRECTORY | O_WRONLY or O_DIRECTORY | O_RDWR at do_last()->may_open().
- open(2) with O_DIRECTORY | O_RDONLY -> fallocate(2) at do_fallocate().

Signed-off-by Rohan Puri <[email protected]>
---
 fs/open.c | 4 ----
 1 file changed, 4 deletions(-)
fs/open.c +0 -4
index 9d64679..5d4f372 100644
@@ -280,10 +280,6 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 	if (S_ISFIFO(inode->i_mode))
 		return -ESPIPE;
 
-	/*
-	 * Let individual file system decide if it supports preallocation
-	 * for directories or not.
-	 */
 	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
 		return -ENODEV;
Signature · 1 line
1.9.1
#2 lore raw

Re: [PATCH 1/2] Remove misleading comment in do_fallocate()

Quoted 23 lines · Rohan Puri

Date: Fri, 18 Jul 2014 15:27:26 +0530
From: Rohan Puri <[email protected]>
To: [email protected], [email protected]
Cc: [email protected]
Subject: [PATCH 1/2] Remove misleading comment in do_fallocate()

Preallocation for directory gets blocked at 2 places & never gets propogated to the underlying file system : -

- open(2) with O_DIRECTORY | O_WRONLY or O_DIRECTORY | O_RDWR at do_last()->may_open().
- open(2) with O_DIRECTORY | O_RDONLY -> fallocate(2) at do_fallocate().

Signed-off-by Rohan Puri <[email protected]>
---
 fs/open.c | 4 ----
 1 file changed, 4 deletions(-)
fs/open.c +0 -0
index 9d64679..5d4f372 100644

fs/open.c int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len) · line 286

Quoted 7 lines
fs/open.c +0 -4
@@ -280,10 +280,6 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 	if (S_ISFIFO(inode->i_mode))
 		return -ESPIPE;
 
-	/*
-	 * Let individual file system decide if it supports preallocation
-	 * for directories or not.
-	 */

What's wrong about this comment ? We're only going to let through regular files and directories and it's up to the file system to decide what it want to support.

-Lukas

Quoted 4 lines
fs/open.c +0 -0
 	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
 		return -ENODEV;

A message that is not in the archive (sent off-list or not yet received): CALJfu6Ok_jdNqacsEKnnVvzxSxCuRC7dA0abve28BASaAgB31A@mail.gmail.com

author
#4 lore raw

PATCH 1/2 Remove misleading comment in do_fallocate()

Adding fs-devel

---------- Forwarded message ----------
From: Rohan Puri <[email protected]>
Date: Fri, Jul 18, 2014 at 4:13 PM
Subject: Re: [PATCH 1/2] Remove misleading comment in do_fallocate()
To: Lukáš Czerner <[email protected]>
Cc: [email protected], [email protected], Linux FS Devel <[email protected]>

Quoted 29 lines · Lukáš Czerner
Quoted 26 lines · Rohan Puri

Date: Fri, 18 Jul 2014 15:27:26 +0530
From: Rohan Puri <[email protected]>
To: [email protected], [email protected]
Cc: [email protected]
Subject: [PATCH 1/2] Remove misleading comment in do_fallocate()

Preallocation for directory gets blocked at 2 places & never gets propogated to the underlying file system : -

- open(2) with O_DIRECTORY | O_WRONLY or O_DIRECTORY | O_RDWR at do_last()->may_open().
- open(2) with O_DIRECTORY | O_RDONLY -> fallocate(2) at do_fallocate().

Signed-off-by Rohan Puri <[email protected]>
---
 fs/open.c | 4 ----
 1 file changed, 4 deletions(-)
fs/open.c +0 -0
index 9d64679..5d4f372 100644
@@ -280,10 +280,6 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
      if (S_ISFIFO(inode->i_mode))
              return -ESPIPE;
Quoted 8 lines

fs/open.c int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len) · line 286

Quoted 4 lines
fs/open.c +0 -4
-     /*
-      * Let individual file system decide if it supports preallocation
-      * for directories or not.
-      */

What's wrong about this comment ? We're only going to let through regular files and directories and it's up to the file system to decide what it want to support.

I agree, but AFAIK for directories we are never going to reach upto the file systems, since directory file opened with O_WRONLY or O_RDWR flag gets blocked by open(2)->do_last()->may_open() and directory with O_RDONLY gets
blocked by do_fallocate() returning -EBADF.
So this comment is misleading in terms that for directory we never go to file systems during fallocate.

Quoted 8 lines

-Lukas

Quoted 4 lines
fs/open.c +0 -0
      if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
              return -ENODEV;

- Rohan
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw

Re: [PATCH 1/2] Remove misleading comment in do_fallocate()

In reply to a message not in the archive
Quoted 44 lines · Rohan Puri

Date: Fri, 18 Jul 2014 16:13:21 +0530
From: Rohan Puri <[email protected]>
To: Lukáš Czerner <[email protected]>
Cc: [email protected], [email protected],
Linux FS Devel <[email protected]>
Subject: Re: [PATCH 1/2] Remove misleading comment in do_fallocate()

Quoted 35 lines · Lukáš Czerner
Quoted 7 lines · Rohan Puri

Date: Fri, 18 Jul 2014 15:27:26 +0530
From: Rohan Puri <[email protected]>
To: [email protected], [email protected]
Cc: [email protected]
Subject: [PATCH 1/2] Remove misleading comment in do_fallocate()

Preallocation for directory gets blocked at 2 places & never gets

propogated

fs/open.c int do_fallocate(struct file *file, int mode,

Quoted 16 lines

to the underlying file system : -

- open(2) with O_DIRECTORY | O_WRONLY or O_DIRECTORY | O_RDWR at do_last()->may_open().
- open(2) with O_DIRECTORY | O_RDONLY -> fallocate(2) at do_fallocate().

Signed-off-by Rohan Puri <[email protected]>
---
 fs/open.c | 4 ----
 1 file changed, 4 deletions(-)
fs/open.c +0 -0
index 9d64679..5d4f372 100644
@@ -280,10 +280,6 @@ int do_fallocate(struct file *file, int mode,

loff_t offset, loff_t len)

Quoted 7 lines
fs/open.c +0 -4
      if (S_ISFIFO(inode->i_mode))
              return -ESPIPE;
 
-     /*
-      * Let individual file system decide if it supports preallocation
-      * for directories or not.
-      */
Quoted 8 lines
Quoted 4 lines

What's wrong about this comment ? We're only going to let through regular files and directories and it's up to the file system to decide what it want to support.

I agree, but AFAIK for directories we are never going to reach upto the file systems, since directory file opened with O_WRONLY or O_RDWR flag gets blocked by open(2)->do_last()->may_open() and directory with O_RDONLY gets blocked by do_fallocate() returning -EBADF.

That's something that needs to be resolved when someone decides to add fallocate support for directories. There has already been a small discussion about that idea.

So I think that the comment is just fine from the fallocate perspective.

-Lukas

Quoted 14 lines

So this comment is misleading in terms that for directory we never go to file systems during fallocate.

Quoted 8 lines

-Lukas

Quoted 4 lines
fs/open.c +0 -0
      if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
              return -ENODEV;

- Rohan

author
#6 lore raw

Re: [PATCH 1/2] Remove misleading comment in do_fallocate()

Quoted 55 lines · Lukáš Czerner
Quoted 44 lines · Rohan Puri

Date: Fri, 18 Jul 2014 16:13:21 +0530
From: Rohan Puri <[email protected]>
To: Lukáš Czerner <[email protected]>
Cc: [email protected], [email protected],
Linux FS Devel <[email protected]>
Subject: Re: [PATCH 1/2] Remove misleading comment in do_fallocate()

Quoted 35 lines · Lukáš Czerner
Quoted 7 lines · Rohan Puri

Date: Fri, 18 Jul 2014 15:27:26 +0530
From: Rohan Puri <[email protected]>
To: [email protected], [email protected]
Cc: [email protected]
Subject: [PATCH 1/2] Remove misleading comment in do_fallocate()

Preallocation for directory gets blocked at 2 places & never gets

propogated

fs/open.c int do_fallocate(struct file *file, int mode,

Quoted 16 lines

to the underlying file system : -

- open(2) with O_DIRECTORY | O_WRONLY or O_DIRECTORY | O_RDWR at do_last()->may_open().
- open(2) with O_DIRECTORY | O_RDONLY -> fallocate(2) at do_fallocate().

Signed-off-by Rohan Puri <[email protected]>
---
 fs/open.c | 4 ----
 1 file changed, 4 deletions(-)
fs/open.c +0 -0
index 9d64679..5d4f372 100644
@@ -280,10 +280,6 @@ int do_fallocate(struct file *file, int mode,

loff_t offset, loff_t len)

Quoted 7 lines
fs/open.c +0 -4
      if (S_ISFIFO(inode->i_mode))
              return -ESPIPE;
 
-     /*
-      * Let individual file system decide if it supports preallocation
-      * for directories or not.
-      */
Quoted 8 lines
Quoted 4 lines

What's wrong about this comment ? We're only going to let through regular files and directories and it's up to the file system to decide what it want to support.

I agree, but AFAIK for directories we are never going to reach upto the file systems, since directory file opened with O_WRONLY or O_RDWR flag gets blocked by open(2)->do_last()->may_open() and directory with O_RDONLY gets blocked by do_fallocate() returning -EBADF.

Quoted 6 lines

That's something that needs to be resolved when someone decides to add fallocate support for directories. There has already been a small discussion about that idea.

So I think that the comment is just fine from the fallocate perspective.

sounds good. Thanks Lukas for clarification :)

Quoted 18 lines

-Lukas

Quoted 15 lines

So this comment is misleading in terms that for directory we never go to file systems during fallocate.

Quoted 8 lines

-Lukas

Quoted 4 lines
fs/open.c +0 -0
      if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
              return -ENODEV;

- Rohan

- Rohan
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#7 lore raw

PATCH 2/2 Remove redundant inode mode checks for ceph & ext4

In reply to Rohan Puri

These checks are not needed here, since they are already performed by do_fallocate()

Signed-off-by Rohan Puri <[email protected]>
---
 fs/ceph/file.c  | 3 ---
 fs/ext4/inode.c | 3 ---
 2 files changed, 6 deletions(-)
fs/ceph/file.c +0 -3
index 88a6df4..687a72e 100644
@@ -1218,9 +1218,6 @@ static long ceph_fallocate(struct file *file, int mode,
 	loff_t endoff = 0;
 	loff_t size;
 
-	if (!S_ISREG(inode->i_mode))
-		return -EOPNOTSUPP;
-
 	mutex_lock(&inode->i_mutex);
 
 	if (ceph_snap(inode) != CEPH_NOSNAP) {
fs/ext4/inode.c +0 -3
index 7fcd68e..22c9868 100644
@@ -3532,9 +3532,6 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
 	unsigned int credits;
 	int ret = 0;
 
-	if (!S_ISREG(inode->i_mode))
-		return -EOPNOTSUPP;
-
 	trace_ext4_punch_hole(inode, offset, length, 0);
 
 	/*
Signature · 1 line
1.9.1
NAK
#8 lore raw

Re: [PATCH 2/2] Remove redundant inode mode checks for ceph & ext4

Quoted 8 lines · Rohan Puri

Date: Fri, 18 Jul 2014 15:28:38 +0530
From: Rohan Puri <[email protected]>
To: [email protected], [email protected]
Cc: [email protected]
Subject: [PATCH 2/2] Remove redundant inode mode checks for ceph & ext4

These checks are not needed here, since they are already performed by do_fallocate()

NAK, they are actually needed because we do not support fallocate on directories here.

-Lukas

Quoted 36 lines
Signed-off-by Rohan Puri <[email protected]>
---
 fs/ceph/file.c  | 3 ---
 fs/ext4/inode.c | 3 ---
 2 files changed, 6 deletions(-)
fs/ceph/file.c +0 -3
index 88a6df4..687a72e 100644
@@ -1218,9 +1218,6 @@ static long ceph_fallocate(struct file *file, int mode,
 	loff_t endoff = 0;
 	loff_t size;
 
-	if (!S_ISREG(inode->i_mode))
-		return -EOPNOTSUPP;
-
 	mutex_lock(&inode->i_mutex);
 
 	if (ceph_snap(inode) != CEPH_NOSNAP) {
fs/ext4/inode.c +0 -3
index 7fcd68e..22c9868 100644
@@ -3532,9 +3532,6 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
 	unsigned int credits;
 	int ret = 0;
 
-	if (!S_ISREG(inode->i_mode))
-		return -EOPNOTSUPP;
-
 	trace_ext4_punch_hole(inode, offset, length, 0);
 
 	/*
author
#9 lore raw

Re: [PATCH 2/2] Remove redundant inode mode checks for ceph & ext4

Quoted 8 lines · Lukáš Czerner
Quoted 6 lines · Rohan Puri

Date: Fri, 18 Jul 2014 15:28:38 +0530
From: Rohan Puri <[email protected]>
To: [email protected], [email protected]
Cc: [email protected]
Subject: [PATCH 2/2] Remove redundant inode mode checks for ceph & ext4

Quoted 6 lines
Quoted 2 lines

These checks are not needed here, since they are already performed by do_fallocate()

NAK, they are actually needed because we do not support fallocate on directories here.

But we never get to file systems for fallocate in directory case, we either return from open() itself
or do_fallocate() before calling f_ops->fallocate()

Quoted 31 lines

-Lukas

Quoted 29 lines
Signed-off-by Rohan Puri <[email protected]>
---
 fs/ceph/file.c  | 3 ---
 fs/ext4/inode.c | 3 ---
 2 files changed, 6 deletions(-)
fs/ceph/file.c +0 -3
index 88a6df4..687a72e 100644
@@ -1218,9 +1218,6 @@ static long ceph_fallocate(struct file *file, int mode,
      loff_t endoff = 0;
      loff_t size;
 
-     if (!S_ISREG(inode->i_mode))
-             return -EOPNOTSUPP;
-
      mutex_lock(&inode->i_mutex);
 
      if (ceph_snap(inode) != CEPH_NOSNAP) {
fs/ext4/inode.c +0 -0
index 7fcd68e..22c9868 100644
@@ -3532,9 +3532,6 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
      unsigned int credits;
      int ret = 0;
Quoted 7 lines

fs/ext4/inode.c int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length) · line 3537

Quoted 7 lines
fs/ext4/inode.c +0 -3
-     if (!S_ISREG(inode->i_mode))
-             return -EOPNOTSUPP;
-
      trace_ext4_punch_hole(inode, offset, length, 0);
 
      /*

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#10 lore raw

Re: [PATCH 2/2] Remove redundant inode mode checks for ceph & ext4

Quoted 19 lines · Rohan Puri

Date: Fri, 18 Jul 2014 16:16:26 +0530
From: Rohan Puri <[email protected]>
To: Lukáš Czerner <[email protected]>
Cc: [email protected], [email protected],
Linux FS Devel <[email protected]>
Subject: Re: [PATCH 2/2] Remove redundant inode mode checks for ceph & ext4

Quoted 11 lines · Lukáš Czerner
Quoted 8 lines · Rohan Puri

Date: Fri, 18 Jul 2014 15:28:38 +0530
From: Rohan Puri <[email protected]>
To: [email protected], [email protected]
Cc: [email protected]
Subject: [PATCH 2/2] Remove redundant inode mode checks for ceph & ext4

These checks are not needed here, since they are already performed by do_fallocate()

Quoted 6 lines
Quoted 3 lines

NAK, they are actually needed because we do not support fallocate on directories here.

But we never get to file systems for fallocate in directory case, we either return from open() itself
or do_fallocate() before calling f_ops->fallocate()

Yes, but this is future proof for the time that someone adds fallocate support for directories. Let's leave it here, it really does not hurt.

Thanks!
-Lukas

Quoted 39 lines
Quoted 38 lines

-Lukas

Quoted 36 lines
Signed-off-by Rohan Puri <[email protected]>
---
 fs/ceph/file.c  | 3 ---
 fs/ext4/inode.c | 3 ---
 2 files changed, 6 deletions(-)
fs/ceph/file.c +0 -3
index 88a6df4..687a72e 100644
@@ -1218,9 +1218,6 @@ static long ceph_fallocate(struct file *file, int mode,
      loff_t endoff = 0;
      loff_t size;
 
-     if (!S_ISREG(inode->i_mode))
-             return -EOPNOTSUPP;
-
      mutex_lock(&inode->i_mutex);
 
      if (ceph_snap(inode) != CEPH_NOSNAP) {
fs/ext4/inode.c +0 -3
index 7fcd68e..22c9868 100644
@@ -3532,9 +3532,6 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
      unsigned int credits;
      int ret = 0;
 
-     if (!S_ISREG(inode->i_mode))
-             return -EOPNOTSUPP;
-
      trace_ext4_punch_hole(inode, offset, length, 0);
 
      /*
author
#11 lore raw

Re: [PATCH 2/2] Remove redundant inode mode checks for ceph & ext4

Quoted 24 lines · Lukáš Czerner
Quoted 22 lines · Rohan Puri

Date: Fri, 18 Jul 2014 16:16:26 +0530
From: Rohan Puri <[email protected]>
To: Lukáš Czerner <[email protected]>
Cc: [email protected], [email protected],
Linux FS Devel <[email protected]>
Subject: Re: [PATCH 2/2] Remove redundant inode mode checks for ceph & ext4

Quoted 14 lines · Lukáš Czerner
Quoted 8 lines · Rohan Puri

Date: Fri, 18 Jul 2014 15:28:38 +0530
From: Rohan Puri <[email protected]>
To: [email protected], [email protected]
Cc: [email protected]
Subject: [PATCH 2/2] Remove redundant inode mode checks for ceph & ext4

These checks are not needed here, since they are already performed by do_fallocate()

NAK, they are actually needed because we do not support fallocate on directories here.

Quoted 8 lines
Quoted 3 lines

But we never get to file systems for fallocate in directory case, we either return from open() itself
or do_fallocate() before calling f_ops->fallocate()

Yes, but this is future proof for the time that someone adds fallocate support for directories. Let's leave it here, it really does not hurt.

Thanks for clarification :)

Quoted 42 lines

Thanks!
-Lukas

Quoted 39 lines
Quoted 38 lines

-Lukas

Quoted 36 lines
Signed-off-by Rohan Puri <[email protected]>
---
 fs/ceph/file.c  | 3 ---
 fs/ext4/inode.c | 3 ---
 2 files changed, 6 deletions(-)
fs/ceph/file.c +0 -3
index 88a6df4..687a72e 100644
@@ -1218,9 +1218,6 @@ static long ceph_fallocate(struct file *file, int mode,
      loff_t endoff = 0;
      loff_t size;
 
-     if (!S_ISREG(inode->i_mode))
-             return -EOPNOTSUPP;
-
      mutex_lock(&inode->i_mutex);
 
      if (ceph_snap(inode) != CEPH_NOSNAP) {
fs/ext4/inode.c +0 -3
index 7fcd68e..22c9868 100644
@@ -3532,9 +3532,6 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
      unsigned int credits;
      int ret = 0;
 
-     if (!S_ISREG(inode->i_mode))
-             return -EOPNOTSUPP;
-
      trace_ext4_punch_hole(inode, offset, length, 0);
 
      /*

- Rohan
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html