On Tue, 24 Mar 2009 17:03:22 -0700 (PDT)
Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
>
> On Tue, 24 Mar 2009, Peter wrote:
> >
> > Thanks a lot , I will check that out tomorrow, in the meantime, this is the
> > result of your patch being applied:
> >
> > $ git add <big stuff>
> >
> > fatal: error when closing sha1 file (Bad file descriptor)
>
> Ok, that's probably cifs_writepages() doing
>
> open_file = find_writable_file(CIFS_I(mapping->host));
> if (!open_file) {
> cERROR(1, ("No writable handles for inode"));
> rc = -EBADF;
> } else {
> ..
>
> so yeah, looks like it's the fchmod() that triggers it.
>
> I suspect this would be a safer - if slightly slower - way to make sure
> the file is read-only. It's slower, because it is going to look up the
> filename once more, but I bet it is going to avoid this particular CIFS
> bug.
>
Yeah, that should work around it. This CIFS patch should also fix it.
It's untested but it's reasonably simple.
Just out of curiosity, what sort of server are you mounting here?
--------------[snip]------------------
>From ea8dc9927fb9542bb1c73b1982cc44bf3d4a9798 Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@xxxxxxxxxx>
Date: Tue, 24 Mar 2009 19:50:17 -0400
Subject: [PATCH] cifs: flush data on any setattr
We already flush all the dirty pages for an inode before doing
ATTR_SIZE and ATTR_MTIME changes. There's another problem though -- if
we change the mode so that the file becomes read-only then we may not
be able to write data to it afterward.
Fix this by just going back to flushing all the dirty data on any
setattr call. There are probably some cases that can be optimized out,
but we need to take care that we don't cause a regression by doing that.
Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
---
fs/cifs/inode.c | 58 ++++++++++++++++++++++++++++--------------------------
1 files changed, 30 insertions(+), 28 deletions(-)
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index a8797cc..f4e880d 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1792,20 +1792,21 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
goto out;
}
- if ((attrs->ia_valid & ATTR_MTIME) || (attrs->ia_valid & ATTR_SIZE)) {
- /*
- Flush data before changing file size or changing the last
- write time of the file on the server. If the
- flush returns error, store it to report later and continue.
- BB: This should be smarter. Why bother flushing pages that
- will be truncated anyway? Also, should we error out here if
- the flush returns error?
- */
- rc = filemap_write_and_wait(inode->i_mapping);
- if (rc != 0) {
- cifsInode->write_behind_rc = rc;
- rc = 0;
- }
+ /*
+ * Attempt to flush data before changing attributes. We need to do
+ * this for ATTR_SIZE and ATTR_MTIME for sure, and if we change the
+ * ownership or mode then we may also need to do this. Here, we take
+ * the safe way out and just do the flush on all setattr requests. If
+ * the flush returns error, store it to report later and continue.
+ *
+ * BB: This should be smarter. Why bother flushing pages that
+ * will be truncated anyway? Also, should we error out here if
+ * the flush returns error?
+ */
+ rc = filemap_write_and_wait(inode->i_mapping);
+ if (rc != 0) {
+ cifsInode->write_behind_rc = rc;
+ rc = 0;
}
if (attrs->ia_valid & ATTR_SIZE) {
@@ -1903,20 +1904,21 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
return -ENOMEM;
}
- if ((attrs->ia_valid & ATTR_MTIME) || (attrs->ia_valid & ATTR_SIZE)) {
- /*
- Flush data before changing file size or changing the last
- write time of the file on the server. If the
- flush returns error, store it to report later and continue.
- BB: This should be smarter. Why bother flushing pages that
- will be truncated anyway? Also, should we error out here if
- the flush returns error?
- */
- rc = filemap_write_and_wait(inode->i_mapping);
- if (rc != 0) {
- cifsInode->write_behind_rc = rc;
- rc = 0;
- }
+ /*
+ * Attempt to flush data before changing attributes. We need to do
+ * this for ATTR_SIZE and ATTR_MTIME for sure, and if we change the
+ * ownership or mode then we may also need to do this. Here, we take
+ * the safe way out and just do the flush on all setattr requests. If
+ * the flush returns error, store it to report later and continue.
+ *
+ * BB: This should be smarter. Why bother flushing pages that
+ * will be truncated anyway? Also, should we error out here if
+ * the flush returns error?
+ */
+ rc = filemap_write_and_wait(inode->i_mapping);
+ if (rc != 0) {
+ cifsInode->write_behind_rc = rc;
+ rc = 0;
}
if (attrs->ia_valid & ATTR_SIZE) {
--
1.6.0.6
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
