* [PATCH 0/4] files: various updates
@ 2005-06-24 10:50 Dipankar Sarma
2005-06-24 10:52 ` [PATCH 1/4] files: fix dupfd by fdt reload Dipankar Sarma
0 siblings, 1 reply; 10+ messages in thread
From: Dipankar Sarma @ 2005-06-24 10:50 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
Andrew,
Here is the set of update patches that fixes, changes and documents
various issues in the new fd management patchset in -mm1. The
first patch - fix-dupfd-reacquire-fdt.patch fixes bugme #4770
and the oopses people other than me were seeing.
I have tested this patchset with 2.6.12-mm1 in my lab, not sure
if it means anything, given bugme #4770 and my userland :-)
Please include these in -mm1 for testing.
Thanks
Dipankar
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] files: fix dupfd by fdt reload
2005-06-24 10:50 [PATCH 0/4] files: various updates Dipankar Sarma
@ 2005-06-24 10:52 ` Dipankar Sarma
2005-06-24 10:53 ` [PATCH 2/4] files: fix expand_files return code Dipankar Sarma
0 siblings, 1 reply; 10+ messages in thread
From: Dipankar Sarma @ 2005-06-24 10:52 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
locate_fd() may expand fdtable, so the fdtable pointer must be
reloaded after locate_fd(). Fixes bugme #4770.
Signed-off-by: Dipankar Sarma <dipankar@in.ibm.com>
---
fs/fcntl.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletion(-)
diff -puN fs/fcntl.c~fix-dupfd-reacquire-fdt fs/fcntl.c
--- linux-2.6.12-mm1-fix/fs/fcntl.c~fix-dupfd-reacquire-fdt 2005-06-25 14:56:58.000000000 +0530
+++ linux-2.6.12-mm1-fix-dipankar/fs/fcntl.c 2005-06-25 14:58:26.000000000 +0530
@@ -118,9 +118,10 @@ static int dupfd(struct file *file, unsi
int fd;
spin_lock(&files->file_lock);
- fdt = files_fdtable(files);
fd = locate_fd(files, file, start);
if (fd >= 0) {
+ /* locate_fd() may have expanded fdtable, load the ptr */
+ fdt = files_fdtable(files);
FD_SET(fd, fdt->open_fds);
FD_CLR(fd, fdt->close_on_exec);
spin_unlock(&files->file_lock);
_
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] files: fix expand_files return code
2005-06-24 10:52 ` [PATCH 1/4] files: fix dupfd by fdt reload Dipankar Sarma
@ 2005-06-24 10:53 ` Dipankar Sarma
2005-06-24 10:54 ` [PATCH 3/4] files: change fd_install assertion Dipankar Sarma
0 siblings, 1 reply; 10+ messages in thread
From: Dipankar Sarma @ 2005-06-24 10:53 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
If expand_fdtable() sees that someone else expanded the fdtable
while it dropped the lock, it can return 0 which in turn
can be returned by expand_files() even though there has
been an expansion of the fdtable since expand_files()
was originally called. This could lead to locate_fd()
not repeating the fd search and returning a bogus fd.
This patch fixes this problem.
Signed-off-by: Dipankar Sarma <dipankar@in.ibm.com>
---
fs/file.c | 15 +++++++--------
1 files changed, 7 insertions(+), 8 deletions(-)
diff -puN fs/file.c~fix-expand-files fs/file.c
--- linux-2.6.12-mm1-fix/fs/file.c~fix-expand-files 2005-06-25 16:42:18.000000000 +0530
+++ linux-2.6.12-mm1-fix-dipankar/fs/file.c 2005-06-25 16:42:18.000000000 +0530
@@ -304,13 +304,14 @@ out:
/*
* Expands the file descriptor table - it will allocate a new fdtable and
* both fd array and fdset. It is expected to be called with the
- * files_lock held.
+ * files_lock held. It returns 1 if fdtable expanded or -errno if
+ * expansion failed.
*/
static int expand_fdtable(struct files_struct *files, int nr)
__releases(files->file_lock)
__acquires(files->file_lock)
{
- int error = 0;
+ int error = 1;
struct fdtable *fdt;
struct fdtable *nfdt = NULL;
@@ -350,7 +351,7 @@ out:
*/
int expand_files(struct files_struct *files, int nr)
{
- int err, expand = 0;
+ int err;
struct fdtable *fdt;
fdt = files_fdtable(files);
@@ -360,11 +361,9 @@ int expand_files(struct files_struct *fi
err = -EMFILE;
goto out;
}
- expand = 1;
- if ((err = expand_fdtable(files, nr)))
- goto out;
- }
- err = expand;
+ err = expand_fdtable(files, nr);
+ } else
+ err = 0;
out:
return err;
}
_
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] files: change fd_install assertion
2005-06-24 10:53 ` [PATCH 2/4] files: fix expand_files return code Dipankar Sarma
@ 2005-06-24 10:54 ` Dipankar Sarma
2005-06-24 10:55 ` [PATCH 4/4] files: doc update Dipankar Sarma
2005-06-24 12:04 ` [PATCH 3/4] files: change fd_install assertion Artem B. Bityuckiy
0 siblings, 2 replies; 10+ messages in thread
From: Dipankar Sarma @ 2005-06-24 10:54 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
Change the fds[fd] != NULL check in fd_install() to be a BUG_ON.
Signed-off-by: Dipankar Sarma <dipankar@in.ibm.com>
---
fs/open.c | 3 +--
1 files changed, 1 insertion(+), 2 deletions(-)
diff -puN fs/open.c~change-fd-install-assert fs/open.c
--- linux-2.6.12-mm1-fix/fs/open.c~change-fd-install-assert 2005-06-25 16:43:02.000000000 +0530
+++ linux-2.6.12-mm1-fix-dipankar/fs/open.c 2005-06-25 16:43:02.000000000 +0530
@@ -931,8 +931,7 @@ void fastcall fd_install(unsigned int fd
struct fdtable *fdt;
spin_lock(&files->file_lock);
fdt = files_fdtable(files);
- if (unlikely(fdt->fd[fd] != NULL))
- BUG();
+ BUG_ON(fdt->fd[fd] != NULL);
rcu_assign_pointer(fdt->fd[fd], file);
spin_unlock(&files->file_lock);
}
_
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] files: doc update
2005-06-24 10:54 ` [PATCH 3/4] files: change fd_install assertion Dipankar Sarma
@ 2005-06-24 10:55 ` Dipankar Sarma
2005-06-24 12:04 ` [PATCH 3/4] files: change fd_install assertion Artem B. Bityuckiy
1 sibling, 0 replies; 10+ messages in thread
From: Dipankar Sarma @ 2005-06-24 10:55 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
Update files documentation to mention the need for reloading
fdtable pointer if ->file_lock is dropped.
Signed-off-by: Dipankar Sarma <dipankar@in.ibm.com>
---
Documentation/filesystems/files.txt | 20 ++++++++++++++++++++
1 files changed, 20 insertions(+)
diff -puN Documentation/filesystems/files.txt~files-doc-update Documentation/filesystems/files.txt
--- linux-2.6.12-mm1-fix/Documentation/filesystems/files.txt~files-doc-update 2005-06-26 06:01:40.000000000 +0530
+++ linux-2.6.12-mm1-fix-dipankar/Documentation/filesystems/files.txt 2005-06-26 06:13:09.000000000 +0530
@@ -101,3 +101,23 @@ the fdtable structure -
API. If they are looked up lock-free, rcu_dereference()
must be used. However it is advisable to use files_fdtable()
and fcheck()/fcheck_files() which take care of these issues.
+
+7. While updating, the fdtable pointer must be looked up while
+ holding files->file_lock. If ->file_lock is dropped, then
+ another thread expand the files thereby creating a new
+ fdtable and making the earlier fdtable pointer stale.
+ For example :
+
+ spin_lock(&files->file_lock);
+ fd = locate_fd(files, file, start);
+ if (fd >= 0) {
+ /* locate_fd() may have expanded fdtable, load the ptr */
+ fdt = files_fdtable(files);
+ FD_SET(fd, fdt->open_fds);
+ FD_CLR(fd, fdt->close_on_exec);
+ spin_unlock(&files->file_lock);
+ .....
+
+ Since locate_fd() can drop ->file_lock (and reacquire ->file_lock),
+ the fdtable pointer (fdt) must be loaded after locate_fd().
+
_
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] files: change fd_install assertion
2005-06-24 10:54 ` [PATCH 3/4] files: change fd_install assertion Dipankar Sarma
2005-06-24 10:55 ` [PATCH 4/4] files: doc update Dipankar Sarma
@ 2005-06-24 12:04 ` Artem B. Bityuckiy
2005-06-24 13:00 ` Dipankar Sarma
1 sibling, 1 reply; 10+ messages in thread
From: Artem B. Bityuckiy @ 2005-06-24 12:04 UTC (permalink / raw)
To: dipankar; +Cc: Andrew Morton, linux-kernel
Dipankar Sarma wrote:
> - if (unlikely(fdt->fd[fd] != NULL))
> - BUG();
> + BUG_ON(fdt->fd[fd] != NULL);
> rcu_assign_pointer(fdt->fd[fd], file);
> spin_unlock(&files->file_lock);
> }
>
Why is this better ?
--
Best regards, Artem B. Bityuckiy
Oktet Labs (St. Petersburg), Software Engineer.
+78124286709 (office) +79112449030 (mobile)
E-mail: dedekind@oktetlabs.ru, web: http://www.oktetlabs.ru
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] files: change fd_install assertion
2005-06-24 12:04 ` [PATCH 3/4] files: change fd_install assertion Artem B. Bityuckiy
@ 2005-06-24 13:00 ` Dipankar Sarma
2005-06-24 13:55 ` Artem B. Bityuckiy
0 siblings, 1 reply; 10+ messages in thread
From: Dipankar Sarma @ 2005-06-24 13:00 UTC (permalink / raw)
To: Artem B. Bityuckiy; +Cc: Andrew Morton, linux-kernel
On Fri, Jun 24, 2005 at 04:04:37PM +0400, Artem B. Bityuckiy wrote:
> Dipankar Sarma wrote:
> >- if (unlikely(fdt->fd[fd] != NULL))
> >- BUG();
> >+ BUG_ON(fdt->fd[fd] != NULL);
> > rcu_assign_pointer(fdt->fd[fd], file);
> > spin_unlock(&files->file_lock);
> > }
> >
> Why is this better ?
Because that way the compare and branch can be ifdefed out when CONFIG_BUG is
not set. Not to mention BUG_ON() looks more like an assertion.
Thanks
Dipankar
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] files: change fd_install assertion
2005-06-24 13:00 ` Dipankar Sarma
@ 2005-06-24 13:55 ` Artem B. Bityuckiy
2005-06-24 14:09 ` Dipankar Sarma
0 siblings, 1 reply; 10+ messages in thread
From: Artem B. Bityuckiy @ 2005-06-24 13:55 UTC (permalink / raw)
To: dipankar; +Cc: Andrew Morton, linux-kernel
Dipankar Sarma wrote:
> Because that way the compare and branch can be ifdefed out when CONFIG_BUG is
> not set. Not to mention BUG_ON() looks more like an assertion.
Surely, even if BUG() will be nothing, the compiler will optimize that?
Yes, it looks better, but I don't like that there was unlikely() before
and you removed it. I'ts minor though.
--
Best regards, Artem B. Bityuckiy
Oktet Labs (St. Petersburg), Software Engineer.
+78124286709 (office) +79112449030 (mobile)
E-mail: dedekind@oktetlabs.ru, web: http://www.oktetlabs.ru
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] files: change fd_install assertion
2005-06-24 13:55 ` Artem B. Bityuckiy
@ 2005-06-24 14:09 ` Dipankar Sarma
2005-06-24 14:20 ` Artem B. Bityuckiy
0 siblings, 1 reply; 10+ messages in thread
From: Dipankar Sarma @ 2005-06-24 14:09 UTC (permalink / raw)
To: Artem B. Bityuckiy; +Cc: Andrew Morton, linux-kernel
On Fri, Jun 24, 2005 at 05:55:21PM +0400, Artem B. Bityuckiy wrote:
> Dipankar Sarma wrote:
> >Because that way the compare and branch can be ifdefed out when CONFIG_BUG
> >is
> >not set. Not to mention BUG_ON() looks more like an assertion.
>
> Surely, even if BUG() will be nothing, the compiler will optimize that?
> Yes, it looks better, but I don't like that there was unlikely() before
> and you removed it. I'ts minor though.
I did not. BUG_ON() is supposed to have unlikely() inside. See the
generic version. If an arch specific BUG_ON() doesn't have some
branch hint, it definitely should.
Thanks
Dipankar
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] files: change fd_install assertion
2005-06-24 14:09 ` Dipankar Sarma
@ 2005-06-24 14:20 ` Artem B. Bityuckiy
0 siblings, 0 replies; 10+ messages in thread
From: Artem B. Bityuckiy @ 2005-06-24 14:20 UTC (permalink / raw)
To: dipankar; +Cc: Andrew Morton, linux-kernel
Dipankar Sarma wrote:
> I did not. BUG_ON() is supposed to have unlikely() inside. See the
> generic version. If an arch specific BUG_ON() doesn't have some
> branch hint, it definitely should.
Ah, pardon.
--
Best regards, Artem B. Bityuckiy
Oktet Labs (St. Petersburg), Software Engineer.
+78124286709 (office) +79112449030 (mobile)
E-mail: dedekind@oktetlabs.ru, web: http://www.oktetlabs.ru
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2005-06-24 14:20 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2005-06-24 10:50 [PATCH 0/4] files: various updates Dipankar Sarma
2005-06-24 10:52 ` [PATCH 1/4] files: fix dupfd by fdt reload Dipankar Sarma
2005-06-24 10:53 ` [PATCH 2/4] files: fix expand_files return code Dipankar Sarma
2005-06-24 10:54 ` [PATCH 3/4] files: change fd_install assertion Dipankar Sarma
2005-06-24 10:55 ` [PATCH 4/4] files: doc update Dipankar Sarma
2005-06-24 12:04 ` [PATCH 3/4] files: change fd_install assertion Artem B. Bityuckiy
2005-06-24 13:00 ` Dipankar Sarma
2005-06-24 13:55 ` Artem B. Bityuckiy
2005-06-24 14:09 ` Dipankar Sarma
2005-06-24 14:20 ` Artem B. Bityuckiy
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.