X-Recipient: archive-cygwin AT delorie DOT com
X-Spam-Check-By: sourceware.org
Date: Mon, 20 Jul 2009 19:49:12 +0200
From: Corinna Vinschen <corinna-cygwin AT cygwin DOT com>
To: cygwin AT cygwin DOT com
Subject: Re: MVFS results
Message-ID: <20090720174912.GT27613@calimero.vinschen.de>
Reply-To: cygwin AT cygwin DOT com
Mail-Followup-To: cygwin AT cygwin DOT com
References: <loom DOT 20090715T192219-408 AT post DOT gmane DOT org> <20090715204831 DOT GA27613 AT calimero DOT vinschen DOT de> <loom DOT 20090715T213245-387 AT post DOT gmane DOT org> <loom DOT 20090717T230701-727 AT post DOT gmane DOT org> <20090718101123 DOT GA8581 AT calimero DOT vinschen DOT de> <loom DOT 20090720T142924-486 AT post DOT gmane DOT org> <20090720153636 DOT GQ27613 AT calimero DOT vinschen DOT de> <loom DOT 20090720T154622-451 AT post DOT gmane DOT org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <loom.20090720T154622-451@post.gmane.org>
User-Agent: Mutt/1.5.19 (2009-02-20)
Mailing-List: contact cygwin-help AT cygwin DOT com; run by ezmlm
Precedence: bulk
List-Id: <cygwin.cygwin.com>
List-Unsubscribe: <mailto:cygwin-unsubscribe-archive-cygwin=delorie DOT com AT cygwin DOT com>
List-Subscribe: <mailto:cygwin-subscribe AT cygwin DOT com>
List-Archive: <http://sourceware.org/ml/cygwin/>
List-Post: <mailto:cygwin AT cygwin DOT com>
List-Help: <mailto:cygwin-help AT cygwin DOT com>, <http://sourceware.org/ml/#faqs>
Sender: cygwin-owner AT cygwin DOT com
Mail-Followup-To: cygwin AT cygwin DOT com
Delivered-To: mailing list cygwin AT cygwin DOT com

On Jul 20 16:58, Eric Blake wrote:
> Corinna Vinschen <corinna-cygwin <at> cygwin.com> writes:
> > +      InitializeObjectAttributes (&attr, &ro_u_empty, pc.objcaseinsensitive 
> (),
> > +				  fh, NULL);
> 
> Yet the obvious fix of using get_handle() instead of fh doesn't help either.  

Ouch, right.  Did it show that I just typed away and didn't test it?

> Before this patch, writable files were able to preserve times (because there 
> was no fchmod in the loop), but after the patch, both writable and read-only 
> are losing the timestamp update.  The duplicated handle does just fine, but 
> then closing the original handle undoes the change:
> [...]
> So, setting the timestamp via closing an alternative handle works, but then 
> we're back to the problem that closing the original handle corrupts the 
> timestamp (and worse, it set it to 10:37 of the handle close, instead of the 
> 10:31 of the file create; whereas before the patch it least left the create 
> time intact).

Oh boy, how broken is that?

> I noticed that http://msdn.microsoft.com/en-us/library/dd852055.aspx states 
> that "However, a driver or application can request that the file system not 
> update one or more of these members for I/O operations that are performed on 
> the caller's file handle by setting the appropriate members to ???1."  Maybe it 
> would work to change the appropriate fields of the original handle to -1 at the 
> same time we close the duplicate handle?  Or will that inhibit correct 
> timestamp modifications from subsequent reads on the open fd?

That's how I understand setting the members to -1.  As far as one can
surmise from the behaviour of MVFS as seen up to here, this would result
in some other unwanted behaviour.  I'm on the verge of giving up on MVFS.

> Another thought - since we are able to see the timestamps updated as soon as we 
> close the duplicated handle, and the fchmod didn't corrupt state (only the 
> subsequent close), maybe we just need to special case close_fs for MVFS to also 
> play games with handles, so that the last handle closed always has the desired 
> times?

I don't like the sound of it.  YA solution which requires to store state
information.  Storing the file attributes in fhandler is already not
quite correct because they can so easily be changed by another piece of
code, another application.

> Or maybe this is a case where we need a FlushFileBuffer call during the 
> utimens_fs after all, in addition to closing the duplicate descriptor, so that 
> the original handle no longer has any state that needs flushing which might 
> inadvertently change timestamps.

That should be handled by the filesystem, in theory.

> I guess my next step will be looking at procmon output, to see if I can make 
> sense of why timestamps are changing as MVFS forwards operations onto the 
> backing store fs.

The easiest solution is probably not to use fchmod on MVFS, but to do
the same trick in fchmod as in utimens:

Index: fhandler_disk_file.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/fhandler_disk_file.cc,v
retrieving revision 1.302
diff -u -p -r1.302 fhandler_disk_file.cc
--- fhandler_disk_file.cc	16 Jul 2009 15:28:57 -0000	1.302
+++ fhandler_disk_file.cc	20 Jul 2009 17:45:08 -0000
@@ -845,7 +845,23 @@ fhandler_disk_file::fchmod (mode_t mode)
   if (S_ISSOCK (mode))
     pc |= (DWORD) FILE_ATTRIBUTE_SYSTEM;
 
-  status = NtSetAttributesFile (get_handle (), pc.file_attributes ());
+  if (pc.fs_is_mvfs () && !oret)
+    {
+      OBJECT_ATTRIBUTES attr;
+      HANDLE fh;
+
+      InitializeObjectAttributes (&attr, &ro_u_empty, pc.objcaseinsensitive (),
+				  get_handle (), NULL);
+      status = NtOpenFile (&fh, FILE_WRITE_ATTRIBUTES, &attr, &io,
+			   FILE_SHARE_VALID_FLAGS, FILE_OPEN_FOR_BACKUP_INTENT);
+      if (NT_SUCCESS (status))
+	{
+	  status = NtSetAttributesFile (fh, pc.file_attributes ());
+	  NtClose (fh);
+	}
+    }
+  else
+    status = NtSetAttributesFile (get_handle (), pc.file_attributes ());
   /* Correct NTFS security attributes have higher priority */
   if (!pc.has_acls ())
     {


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple