On Thu, May 11, 2017 at 2:28 AM, Charan Reddy G <reddychara...@gmail.com> wrote:
> @JV and @Sijie I think Android java source is not the appropriate one to > look into. For FileChannelImpl we need to look into jdk code > > http://grepcode.com/file/repository.grepcode.com/java/ > root/jdk/openjdk/8u40-b25/sun/nio/ch/FileChannelImpl.java# > FileChannelImpl.force%28boolean%29 > > > public void force(boolean metaData) throws IOException { > ensureOpen(); > int rv = -1; > int ti = -1; > try { > begin(); > ti = threads.add(); > if (!isOpen()) > return; > do { > rv = nd.force(fd, metaData); > } while ((rv == IOStatus.INTERRUPTED) && isOpen()); > } finally { > threads.remove(ti); > end(rv > -1); > assert IOStatus.check(rv); > } > } > > here 'nd' is of type FileDispatcherImpl. I tried to look into its code but > since it is native code, we wont be able to read that > > http://grepcode.com/file/repository.grepcode.com/java/ > root/jdk/openjdk/8u40-b25/sun/nio/ch/FileDispatcherImpl. > java#FileDispatcherImpl > > So it is not clear if it is just difference of fdatasync() and fsync(). > Even if we assume that it is the difference of fdatasync() and fsync(), > man pages of fdatasync() says > > "fdatasync() is similar to fsync(), but does not flush modified metadata > unless that metadata is needed in order to allow a subsequent data > retrieval to be correctly handled. For example, changes to st_atime > or st_mtime (respectively, time of last access and time of last > modification; see stat(2)) do not require flushing because they are not > necessary for a subsequent data read to be handled correctly. *On the > other hand, a change to the file size (st_size, as made by say > ftruncate(2)), would require a metadata flush.**"* > > fdatasync - "does not flush modified metadata unless that metadata is needed in order to allow a subsequent data retrieval to be correctly handled" In practice, this means that unless the file size changed, the file metadata will not be synced when calling fdatasync. > Also, when we look into java API for force method - > https://docs.oracle.com/javase/7/docs/api/java/nio/ > channels/FileChannel.html#force(boolean), it says the implementation is > unspecified and system-dependent. So I don't think it is safe to make any > assumptions here. > > "The metaData parameter can be used to limit the number of I/O operations > that this method is required to perform. Passing false for this parameter > indicates that only updates to the file's content need be written to > storage; passing true indicates that updates to both the file's content > and metadata must be written, which generally requires at least one more > I/O operation. Whether this parameter actually has any effect is dependent > upon the underlying operating system and is therefore unspecified. Invoking > this method may cause an I/O operation to occur even if the channel was > only opened for reading. Some operating systems, for example, maintain a > last-access time as part of a file's metadata, and this time is updated > whenever the file is read. Whether or not this is actually done is > system-dependent and is therefore unspecified." > > Thanks, > Charan > > On Wed, May 10, 2017 at 4:07 AM, Sijie Guo <guosi...@gmail.com> wrote: > >> >> >> On May 10, 2017 12:42 PM, "Venkateswara Rao Jujjuri" <jujj...@gmail.com> >> wrote: >> >> Looked at the source and it is fdatasync() >> https://android.googlesource.com/platform/libcore/+/2496a68/ >> luni/src/main/java/java/nio/FileChannelImpl.java >> if (metadata) { >> Libcore.os.fsync(fd); >> } else { >> Libcore.os.fdatasync(fd); >> } >> >> So, agreed. fdatasync() fluesh metadata when the file size etc changes, >> and avoids metadata flush if no data related changes but >> but that tells me there may be no situation for us to call force(true) in >> the code. >> But our code calls it with true in some code path, should we change >> everything to false for consistency >> >> >> I don't remember that we called force(true) on file channel. Do you mean >> the buffered channel? >> >> >> JV >> >> On Tue, May 9, 2017 at 4:40 PM, Sijie Guo <guosi...@gmail.com> wrote: >> >>> We don't force flush metadata in bookies. Because: >>> >>> - flush metadata requires more I/O operations. >>> - flush metadata usually updates file metadata information (e.g. access >>> permissions, last modification/access time). >>> >>> The difference of force(false) and force(true) is the difference between >>> fdatasync() and fsync(). The former one update last modification time, >>> while the latter one doesn't. We don't care about the metadata information >>> like access/modification time, that's the reason we only called >>> force(false). >>> >>> - Sijie >>> >>> >>> On Sat, May 6, 2017 at 9:10 AM, Charan Reddy G <reddychara...@gmail.com> >>> wrote: >>> >>>> By metadata I mean metadata of file >>>> >>>> in BufferedChannel.java >>>> >>>> public void flush(boolean shouldForceWrite) throws IOException { >>>> synchronized(this) { >>>> flushInternal(); >>>> } >>>> if (shouldForceWrite) { >>>> forceWrite(false); <------------- false here >>>> } >>>> } >>>> >>>> public long forceWrite(boolean forceMetadata) throws IOException { >>>> // This is the point up to which we had flushed to the file >>>> system page cache >>>> // before issuing this force write hence is guaranteed to be >>>> made durable by >>>> // the force write, any flush that happens after this may or may >>>> // not be flushed >>>> long positionForceWrite = writeBufferStartPosition.get(); >>>> fileChannel.force(forceMetadata); <--------------- >>>> forcemetadata false >>>> return positionForceWrite; >>>> } >>>> >>>> On Fri, May 5, 2017 at 5:56 PM, Charan Reddy G <reddychara...@gmail.com >>>> > wrote: >>>> >>>>> Hey, >>>>> >>>>> In GarbageCollectorThread.doCompactEntryLogs { >>>>> >>>>> ... >>>>> >>>>> try { >>>>> compactEntryLog(scannerFactory, meta); >>>>> scannerFactory.flush(); <--------------- >>>>> this will eventually call entrylogger.flushCurrentLog and it force writes >>>>> the content of the BufferedChannel but not the metadata of the file >>>>> >>>>> LOG.info("Removing entry log {} after compaction", >>>>> meta.getEntryLogId()); >>>>> removeEntryLog(meta.getEntryLogId()); <----------- >>>>> this will delete the compacted entrylog >>>>> >>>>> } >>>>> ... >>>>> >>>>> >>>>> in doCompactEntryLogs, we first write the non-deleted entries of the >>>>> entryLog which is getting compacted to the currentLog in entryLogger, then >>>>> we flush the entrylogger before deleting the compacted entrylog. But when >>>>> currentLog is flushed by the entrylogger, it flushes only the content of >>>>> the file but not the metadata. After flush is completed the compacted >>>>> entrylog is deleted. It is not ok to not to force flush the metadata of >>>>> the >>>>> currentLog for the persisted (checkpointed) data. The filesystem behaviour >>>>> is unexpected in this case and there is possibility of data loss if the >>>>> Bookie crashes before closing that logchannel. >>>>> >>>>> Thanks, >>>>> Charan >>>>> >>>> >>>> >>> >> >> >> -- >> Jvrao >> --- >> First they ignore you, then they laugh at you, then they fight you, then >> you win. - Mahatma Gandhi >> >> >> >> >
