Bug 225767 – Introduce FileSystem::hardLinkCount()

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Chris Dumez]

Introduce FileSystem::hardLinkCount() to replace our platform-specific implementation in NetworkCacheBlobStorage.

[Chris Dumez]

Created attachment 428520 [details]
Patch

[Chris Dumez]

Created attachment 428524 [details]
Patch

[Chris Dumez]

Created attachment 428529 [details]
Patch

[Chris Dumez]

Created attachment 428530 [details]
Patch

[Darin Adler]

Comment on attachment 428530 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428530&action=review

> Source/WTF/wtf/FileSystem.h:172
> +WTF_EXPORT_PRIVATE Optional<uint64_t> hardLinkCount(const String& path);

We could consider in future using Expected instead of Optional here if callers might need to know what specific error occurred.

> Source/WebKit/NetworkProcess/cache/NetworkCacheBlobStorage.cpp:71
> +        if (linkCount && *linkCount == 1)
> +            FileSystem::deleteFile(path);

Does this single failure of hardLinkCount orphan a file forever and mean it will never get cleaned up? Obviously not new to this refactoring.

[Chris Dumez]

(In reply to Darin Adler from comment #5)
> Comment on attachment 428530 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428530&action=review
> 
> > Source/WTF/wtf/FileSystem.h:172
> > +WTF_EXPORT_PRIVATE Optional<uint64_t> hardLinkCount(const String& path);
> 
> We could consider in future using Expected instead of Optional here if
> callers might need to know what specific error occurred.
> 
> > Source/WebKit/NetworkProcess/cache/NetworkCacheBlobStorage.cpp:71
> > +        if (linkCount && *linkCount == 1)
> > +            FileSystem::deleteFile(path);
> 
> Does this single failure of hardLinkCount orphan a file forever and mean it
> will never get cleaned up? Obviously not new to this refactoring.

Antti is in cc and I believe he wrote this code. I am not sure how to best deal with this so I am not planning to address it in this patch.

There could be several issues for hardLinkCount() to fail:
1. The file doesn't exist: No harm in this case not to delete the file
2. Permission issue: What are the odds deleteFile() is going to succeed then?

Also, if we fail to get the hardlink count, there is really no way for sure for us to know the file is not useful anymore and is safe to be deleted.

[Darin Adler]

(In reply to Chris Dumez from comment #6)
> 1. The file doesn't exist: No harm in this case not to delete the file

Agreed.

> 2. Permission issue: What are the odds deleteFile() is going to succeed then?

The question for us to ask is: Why would this happen in practice? What would we want for a user affected by this?

> Also, if we fail to get the hardlink count, there is really no way for sure
> for us to know the file is not useful anymore and is safe to be deleted.

Absolutely, that’s right.

We still have to ask the bigger question and figure out if there’s any issue here that has to be dealt with; that doesn’t necessarily mean this code would change.

[EWS]

Committed r277446 (237694@main): <https://commits.webkit.org/237694@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428530 [details].

[Radar WebKit Bug Importer]

<rdar://problem/77981837>

[Antti Koivisto]

> Does this single failure of hardLinkCount orphan a file forever and mean it
> will never get cleaned up? Obviously not new to this refactoring.

No, we'll just try again on next synchronize().

The most likely reason this would fail is probably that the file already got deleted (by another instance of NetworkProcess running in the same cache directory for example). This is ok.